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
to
fix all warnings. Is it possible to limit check only to some files?

Yes, you can include/exclude files which are checked, see
https://github.com/bbatsov/rubocop/blob/master/README.md#includingexcluding-files

So you could include "lib/*" (I guess the most of the new code will be there)
and
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
automatically...

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

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`

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
that
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
https://github.com/bbatsov/ruby-style-guide#underscore-unused-vars

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.

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
(https://github.com/bbatsov/ruby-style-guide#lambda-multi-line)
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....

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.

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

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?

Again, I'll recheck this...


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.

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

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.


--

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

< Previous Next >
Follow Ups