On Mon, 24 Nov 2014 19:56:00 +0100 Ladislav Slezak <lslezak@suse.cz> wrote:
Hi all,
I looked at Rubocop (a Ruby code scanner) [1] and tried to use it in the registration module to see if it could be used in Yast to improve the code quality.
I have summarized my findings in a blog post [2], in short:
- it is highly configurable - it can find also some semantic issues - it suggests how to fix the found issues - it can auto correct many issues (esp. indentation and white space)
You can see the found issues and fixes in this pull request [3]. Many issues were just harmless indentation or white space issues, but some were quite critical like this "private" attribute misuse [4]. Majority of the issues were autocorrected by Rubocop itself, I just reviewed the changes and committed each fix in a separate commit to see what was the problem and how it was fixed.
What do you think about Rubocop and Yast? Would it make sense to use it globally for all modules? What would be the pros and cons for you? Would you like to fix the coding issues in the existing code?
Thanks for your feedback!
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? 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. I also considering code that use ruby C-part that maybe need to use methods from such binding that does not follow conventions, if it is possible to disable check there. 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 ). I also found quite confusing this commit https://github.com/yast/yast-registration/commit/f0882e005ede24e7df747f803f2... why not simple `expect(fp1).not_to eq nil` ? https://github.com/yast/yast-registration/commit/f1380e4586897aaa23cfb20ab0f... similar here. I expect something like `expect(fp1).to eq fp1` 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 ( I mean this commit https://github.com/yast/yast-registration/commit/42a9b37e83540e4c93baf53abd5... ) 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/b814d2b7b1351343cc71ad6ad9e... similar question for aligning function calls. If you have Align plugin in vim, it is really trivial to make it aligned. Is there reason to use new lambda syntax? I found it harder to read then lambda one. When using lambda it is for me tight with lambda calculus, syntax with ->() is quite confusing for me and not so easy to follow. https://github.com/yast/yast-registration/commit/8c9b333fcc347335da94196d6c9... I also think that we should not disable unless check https://github.com/yast/yast-registration/commit/fc7efc4dc8e294cb79574e4c3f8... 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. 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. https://github.com/yast/yast-registration/commit/19df8cbd13e677bfb52384f2959... when I see this change ( https://github.com/yast/yast-registration/commit/3e89195d3938d04c3de39b2a5a9... ), 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? This commit is interesting: https://github.com/yast/yast-registration/commit/9c41a8eb25f6ee1da0bade91eb8... 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. 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 ). Also I think we should document all style rules we use as there is a lot of decisions in pull request. 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. Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org