Mailinglist Archive: yast-devel (108 mails)

< Previous Next >
Re: [yast-devel] Yast and Rubocop?
On Tue, 25 Nov 2014 13:14:32 +0100
Ladislav Slezak <lslezak@xxxxxxx> wrote:

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.

Thanks for link. It looks good.


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

wide screen LCD do not help you in textmode with 80 chars width
terminal. Also it is not so easy to get code that is on so long line.
In general need for such long line is code smell :)


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?

yes, as you only use it in assign.


The upstream style guide on the other hand prefers prefixing to plain
_, see
https://github.com/bbatsov/ruby-style-guide#underscore-unused-vars

OK, it depends what you need. In some case it make sense, if it is not
obvious what is other parts of response.


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

If there is problem if would be nice to document what exactly.


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


It can be intersting how others see it.

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 :-)


Ah, ok, it make sense.

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.

It make sense. For old code add checks for ruby style problems that is
not in translated code. And for new more checks, that can even conflict
with translated code like


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.

agreed. But it would be good to define goal which we want reach. Like
complexity smaller then 7 do not make sense.


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.


Yes, it would be nice.

I probably try to configure it for bootloader and see what I see.

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

< Previous Next >