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

[Dan Duvall]

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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.wikimedia.org/pipermail/qa/attachments/20141125/4ceb51f9/attachment.html>