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 <[email protected]> wrote: > On Tue, Nov 25, 2014 at 11:33 AM, S Page <[email protected]> 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>
_______________________________________________ QA mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/qa
