Mailinglist Archive: yast-devel (108 mails)

< Previous Next >
Re: [yast-devel] Yast and Rubocop?
On Mon, 24 Nov 2014 19:56:00 +0100
Ladislav Slezak <lslezak@xxxxxxx> wrote:


Hi all,


I looked at Rubocop (a Ruby code scanner) [1] and tried to use it in
the registration module to see if it could be used in Yast to improve
the code quality.

I have summarized my findings in a blog post [2], in short:

- it is highly configurable
- it can find also some semantic issues
- it suggests how to fix the found issues
- it can auto correct many issues (esp. indentation and white space)


You can see the found issues and fixes in this pull request [3]. Many
issues were just harmless indentation or white space issues, but some
were quite critical like this "private" attribute misuse [4].
Majority of the issues were autocorrected by Rubocop itself, I just
reviewed the changes and committed each fix in a separate commit to
see what was the problem and how it was fixed.


What do you think about Rubocop and Yast? Would it make sense to use
it globally for all modules? What would be the pros and cons for you?
Would you like to fix the coding issues in the existing code?


Thanks for your feedback!

I think it is really interesting, but for bigger modules it is quite
lot work to fix all warnings. Is it possible to limit check only to
some files? Like when I do refactoring in bootloader, I want to have all
refactored code to follow ruby conventions, but old code still need
cleaning, so it do not make much sense to do all changes just to throw
it away when I refactor it.
I also considering code that use ruby C-part that maybe need to
use methods from such binding that does not follow conventions, if it is
possible to disable check there.

I also see that you set line lenght to 140 characters. This looks too
much for me, especially if we need to modify code for debuging
purpose during installation where is limited console. But we can use it
similar like metrics ( see below ).

I also found quite confusing this commit
https://github.com/yast/yast-registration/commit/f0882e005ede24e7df747f803f2df6d6a160fa79
why not simple `expect(fp1).not_to eq nil` ?

https://github.com/yast/yast-registration/commit/f1380e4586897aaa23cfb20ab0f64b4c548b9f6b
similar here. I expect something like `expect(fp1).to eq fp1`

Also I am not sure if it is good idea to name attributes that is
throwed out. For me better convention is to use simple "_" for such
variables, so it is clear that we do not care:

_, _, a, _ = [0,1,2,3]
a
=> 2

( I mean this commit
https://github.com/yast/yast-registration/commit/42a9b37e83540e4c93baf53abd501b191333dae4
)

Another question why do you disable align of Arrays and Hashes?
Converted code use it and I found it quite helpful when reading code:
https://github.com/yast/yast-registration/commit/b814d2b7b1351343cc71ad6ad9e30cd1e15383b9

similar question for aligning function calls. If you have Align plugin
in vim, it is really trivial to make it aligned.

Is there reason to use new lambda syntax? I found it harder to read
then lambda one. When using lambda it is for me tight with lambda
calculus, syntax with ->() is quite confusing for me and not so easy
to follow.
https://github.com/yast/yast-registration/commit/8c9b333fcc347335da94196d6c9104140b72e50f

I also think that we should not disable unless check
https://github.com/yast/yast-registration/commit/fc7efc4dc8e294cb79574e4c3f88275bedb047c7
it is often hard to read and I think also in suse style guide we have
using unless only in trailing form and only with single argument,
otherwise it is quite hard to read it as you need to think how logical
operators works with global negotiation. For me it is more familiar to
use if with negotiation in more complex expressions.

Also what is reason to not check extra indentation for multiline
operators? I think it really helps with code readability and with
coupling lines together.
https://github.com/yast/yast-registration/commit/19df8cbd13e677bfb52384f2959f7cf1ee994a1a

when I see this change
(
https://github.com/yast/yast-registration/commit/3e89195d3938d04c3de39b2a5a911daf4d74fe08#diff-10fd72d29294370773d49226a6b02eefL163
), I am not sure if it helps with readability.
I think better change is to change it to

if ret == :skip && confirm_skipping
log.info "Skipping registration on user request"
@registration_skipped = true
break
end
end

in general I am not sure if it is wrong to use return in loops. What is
rationale behind?


This commit is interesting:
https://github.com/yast/yast-registration/commit/9c41a8eb25f6ee1da0bade91eb813c68cd88032f

I think it can help to set it to max number that pass to ensure we do
not increase complexity when changing code and when refactoring
then decrease numbers, so we are sure we are going forward with our goal
with improved code and do not make steps back.


In general I think it is really interesting project and what we need is
agreement which rules we want to follow and if we want follow same
think for all modules or if we allow module specific checks ( like
method names only for specific modules ). Also I think we should
document all style rules we use as there is a lot of decisions in pull
request.

What I really like is that automatic checks allow us to not go back
with our goal to improve code quality/ ruby style compliance. So if
someone else need to touch your code, (s-)he cannot make it worse.

Josef
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
References