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 [email protected] SUSE -- To unsubscribe, e-mail: [email protected] To contact the owner, e-mail: [email protected]
