Mailinglist Archive: yast-devel (108 mails)

< Previous Next >
Re: [yast-devel] Yast and Rubocop?
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
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
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
follow ruby conventions, but old code still need cleaning, so it do not make
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
enable only the checks which can be autocorrected and run autocorrection to fix

There are many possibilities how to deal with the old code, see the doc link

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
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...

I also found quite confusing this commit

why not simple `expect(fp1).not_to eq nil` ?

similar here. I expect something like `expect(fp1).to eq fp1`

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.
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 that....

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
complex expressions.

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 "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

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
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
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
and one less strict for the legacy (converted) code.

We will need to adapt the config for each repository anyway, for example the
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
(and adapt our style guide if needed).

What I really like is that automatic checks allow us to not go back with our
to improve code quality/ ruby style compliance. So if someone else need to
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


Ladislav Slez√°k
Appliance department / YaST Developer
Lihovarsk√° 1060/12
190 00 Prague 9 / Czech Republic
tel: +420 284 028 960
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
Follow Ups