search

leikind / wice_grid

A Rails grid plugin to create grids with sorting, pagination, and (automatically generated) filters
MIT License
536 stars 215 forks source link

Muffles RuboCop warnings, while leaving roadmap for future work. #300

Closed jdickey closed 8 years ago

jdickey commented 8 years ago

RuboCop is a virtually industry-standard tool for verifying that Ruby source files comply with community-originated Ruby language-usage conventions. These conventions have been arrived at over the years as an aid in improving the overall quality of maintained code.

However, introducing a tool such as RuboCop to an existing, legacy code base commonly yields numerous violation messages, presenting an apparently daunting task to the legacy maintainers. RuboCop has an established convention for dealing with such situations; the standard .rubocop.yml file is created with the single line inherit_from: .rubocop_todo.yml. That latter file, produced by running rubocop --auto-gen-config, is the result of analysing the existing code and, for each "cop" or convention test known to RuboCop, explicitly preventing files found to be in violation from raising violation notices. Thus, the existing code base gets an apparent "clean bill of health"; RuboCop may then be included in the default Rake task flow without problems. As files are modified going forward, the .rubocop_todo.yml file should be consulted for mentions of that file. As developer time permits, mentions of that source file in the todo config should be individually removed, and the resulting violations cleaned up.

In that manner, an existing code base can gradually be brought into compliance with conventions that many, if not most, Ruby OSS developers are familiar with. This has the proven effect of reducing defect count and developer workload as the "cleanliness" of the initially typically wildly non-compliant codebase is brought up to snuff. All the while, this does not materially add to the overall work required of the core maintainers, as existing code "passes" inspection and any new changes which introduce questionable code are called out immediately when the code is still fresh in the mind of the developer who wrote that new code.

The result is a typically extremely significant quality win for the maintainer team, and a reassurance to project teams considering the use of the subject code (such as this Gem) for their own projects. What's not to like?

As an example of incremental repair, the method Wice::Columns#yield_declaration has had a guard clause introduced, returning immediately if the value returned by calling #yield_declaration_of_column_filter is nil and eliminating the need for a previous all-encompassing if block. The file has correspondingly been removed from the Style/GuardClause exclusion list in .rubocop_todo.yml. Total time to make the change and verify that the spec suite still gave a green bar: less than a minute.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 38.66% when pulling 9e47984b09ef53a383f6ed512fd1ec72c4ca33f4 on jdickey:muffle-rubocop into 8002703a0c76ea7ad05710abfe781ed37ceff040 on leikind:rails3.

jdickey commented 8 years ago

Pinging #298 and #299.

leikind commented 8 years ago

Sorry, I am not approving of yet another Ruby cargo cult. Honestly, I don't care, anyone can use whatever floats one's boat, but creating PRs which focus exclusively on tools without solving any real problems that the plugin has is not programming, it is an illusion of programming. I've seen too many developers who spend their days doing nothing but speeding up tests, rewriting tests, dancing around tools like Rubocop, pursuing idiotic fetishes like single responsibility principle ... and not solving (or being able to solve) any real problems. This is nothing but cargo cult and programming masturbation.

Also, words like industry-standard tool don't impress me, I've played enough with Rubocop and I've seen too many plain stupid cops to take words industry-standard tool seriously. Yes, there are some really useful cops, too, but it doesn't change what I've said.

Ruby needs a built-in tool like go fmt

noahc commented 8 years ago

If anyone is curious what @leikind means by real problems, checking out https://github.com/leikind/wice_grid/blob/rails3/TODO.md might be useful.

I hope to try to tackle the removal of Kaminari, but honestly, I don't know the code base well enough to make it happen quickly.