Dne 25.11.2014 v 10:51 Josef Reidinger napsal(a):
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?
Yes, you can include/exclude files which are checked, see
So you could include "lib/*" (I guess the most of the new code will be there)
exclude the rest (you can even list specific files). Or you could enable just some
checks for the old code, like check indentation and disable checks which require
variable or method renames or some refactoring.
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.
You could also disable warnings and just check for errors (or worse) or you could
enable only the checks which can be autocorrected and run autocorrection to fix them
There are many possibilities how to deal with the old code, see the doc link above.
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 ).
Ok, that's probably too much... What about 120? Personally I think the 80 chars
limit does not make much sense with wide screen LCDs anymore...
Oh, good point, that's a better fix, thanks!
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
we do not care:
_, _, a, _ = [0,1,2,3] a => 2
Um, I was little bit worried here about possible conflict with _ function (_() is a
gettext translation function). Is it safe? In all contexts?
The upstream style guide on the other hand prefers prefixing to plain _, see
Another question why do you disable align of Arrays
and Hashes? Converted code
use it and I found it quite helpful when reading code:
similar question for aligning function calls. If you have Align plugin in vim, it
is really trivial to make it aligned.
Um, there was some problem with it, I'll look at it again...
Is there reason to use new lambda syntax?
Not really, our style guide does not mention this and the upstream style guide
suggests using ->() for single line functions and "lambda" for multi-line.
My approach here is: if it is not in our style guide, lets use the upstream one.
And if we don't like the upstream default then adapt our style...
I'm not strict here, if we decide to not use ->() at all I'm fine with
I also think that we should not disable unless check
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
The check works the other way round, it *requires* to use "unless"
instead of "if !" so disabling that check is OK :-)
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.
I'll recheck this...
when I see this change (
), 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
Again, I'll recheck this...
This commit is interesting:
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.
Yes, these numbers were automatically set to the highest found and should be
gradually decreased with each refactoring.
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
Maybe we should create two default configs, one more strict for new modules/files
and one less strict for the legacy (converted) code.
We will need to adapt the config for each repository anyway, for example the maximum
code complexity or max. method length will be different in each repo.
Also I think we should document all style rules we use
as there is a lot of
decisions in pull request.
Yes, once we agree on the final checks and style we should document our decisions.
(and adapt our style guide if needed).
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.
Yes and these checks will help us to avoid nitpicking comments in reviews as
the style checks will be already done by Rubocop ;-)
And if we use the same Rubocop config file (or base it on the same) in all
repositories we can ensure that the coding style is uniform across all Yast modules.
Appliance department / YaST Developer
190 00 Prague 9 / Czech Republic
tel: +420 284 028 960
To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org
To contact the owner, e-mail: yast-devel+owner(a)opensuse.org