[QA] Ruby Coding Conventions and RuboCop (was: Rubocop style checker)

[Dan Duvall]

Yay, my pull request was merged! [1] \o/ Upon the next release we can
simplify our base RuboCop configuration to use `AllCops/StyleGuideCopsOnly:
true` which should greatly reduce our run-ins with overzealous RuboCop law
enforcement.

[1] https://github.com/bbatsov/rubocop/pull/1454

On Tue, Nov 25, 2014 at 12:25 PM, Dan Duvall <dduvall at wikimedia.org> wrote:

> On Tue, Nov 25, 2014 at 11:33 AM, S Page <spage at wikimedia.org> wrote:
>
>> The last rubocop failure in Flow is
>>
>> Offenses: tests/browser/features/support/pages/flow_page.rb:5:1: C: Class definition is too long. [169/165] class FlowPage < WikiPage
>>
>> enforced by: .rubocop_todo.yml's
>> Metrics/ClassLength:
>>   Max: 165
>>
>> 1. Do we want to enforce a max class length or not?  The suggested base
>> configuration in
>> https://www.mediawiki.org/wiki/Manual:Coding_conventions/Ruby#RuboCop
>> disables it:
>>
>> Metrics/ClassLength:
>>   Enabled: false
>>
>>
> That's one of a handful of RuboCop rules that's: 1) not actually in the
> style guide; and 2) sort of arbitrary in where it draws the line. (i.e.
> It's more important that a class follow the Single Responsibility Principle
> than be under some arbitrary number of lines in its implementation.) For
> those reasons, the coding conventions suggest ignoring those rules (for now
> anyway, the debate is always open).
>
> On a related note, I've submitted a pull request to RuboCop that allows
> for invocation that's more in line with the style guide recommendations.[1]
>
> [1] https://github.com/bbatsov/rubocop/pull/1454
>
> 2. Yes, the flow_page class is big, because it defines a lot of page
>> elements. I would like separate groups in the file for flow_topic_buttons,
>> flow_moderation_dialog, etc. because our recommendation[1] to alphabetize
>> it pushes these groups far apart. Is there an example of refactoring a page
>> object that would make sense to a r00by nuby?
>>
>
> See my comments above but that said, if you feel that the class could be
> broken up to better reflect a division of responsibility, I would say:
> follow that inclination. The only rule of style I do suggest following is
> that each class definition reside in its own file.[2]
>
> [2] https://github.com/bbatsov/ruby-style-guide#one-class-per-file
>
> There are some options for separating out the implementation of a single
> class (into modules) but it might be a little tricky with page objects
> since they use class methods (aka macros) to define elements.
>
> Essentially, we may be able to turn this ...
>
> class FlowPage
>   include PageObject
>
>   # Collapse button
>   a(:topics_and_posts_view, href: "#topics/full")
>   a(:small_topics_view, href: "#topics/compact")
>   a(:topics_only_view, href: "#topics/topics")
>
>   # Dialogs
>   div(:dialog, css: ".flow-ui-modal")
>   textarea(:dialog_input, name: "topic_reason")
>   button(:dialog_cancel, css: "a.mw-ui-destructive:nth-child(2)")
>   button(:dialog_submit_delete, text: "Delete")
>   button(:dialog_submit_hide, text: "Hide")
>   button(:dialog_submit_suppress, text: "Suppress")
>
>   # ...
> end
>
> ... into something like this ...
>
> class FlowPage
>   include PageObject
> end
>
> module FlowButtons
>   include PageObject
>
>   a(:topics_and_posts_view, href: "#topics/full")
>   a(:small_topics_view, href: "#topics/compact")
>   a(:topics_only_view, href: "#topics/topics")
> end
>
> module FlowDialogs
>   include PageObject
>
>   div(:dialog, css: ".flow-ui-modal")
>   textarea(:dialog_input, name: "topic_reason")
>   button(:dialog_cancel, css: "a.mw-ui-destructive:nth-child(2)")
>   button(:dialog_submit_delete, text: "Delete")
>   button(:dialog_submit_hide, text: "Hide")
>   button(:dialog_submit_suppress, text: "Suppress")
> end
>
> ... but I'm not 100% if that's the right implementation. If you think that
> kind of separate would be appropriate, let me know and we'll pair on it! :)
>
> --
> Dan Duvall
> Automation Engineer
> Wikimedia Foundation <http://wikimediafoundation.org>
>



-- 
Dan Duvall
Automation Engineer
Wikimedia Foundation <http://wikimediafoundation.org>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.wikimedia.org/pipermail/qa/attachments/20141205/36a6684a/attachment.html>