<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 25, 2014 at 11:33 AM, S Page <span dir="ltr"><<a href="mailto:spage@wikimedia.org" target="_blank">spage@wikimedia.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>The last rubocop failure in Flow is<br><pre><span></span>Offenses:
<span> </span>tests/browser/features/support/pages/flow_page.rb:5:1: C: Class definition is too long. [169/165]
<span> </span>class FlowPage < WikiPage<br></pre></div><div>enforced by:<font> </font>.rubocop_todo.yml's<br></div><font><span style="font-family:'courier new',monospace"></span></font><div><span style="font-family:'courier new',monospace"><font>Metrics/ClassLength:<br> Max: 165</font><br></span><br></div><div>1. Do we want to enforce a max class length or not? The suggested base configuration in <a href="https://www.mediawiki.org/wiki/Manual:Coding_conventions/Ruby#RuboCop" target="_blank">https://www.mediawiki.org/wiki/Manual:Coding_conventions/Ruby#RuboCop</a> disables it:<br><pre><span><font>Metrics/ClassLength</font></span><font>:<span>
Enabled</span><span>: </span>false</font></pre></div></div></blockquote><div><br></div><div>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).<br></div><div><div><br></div><div>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]</div></div><div><br></div><div><div>[1] <a href="https://github.com/bbatsov/rubocop/pull/1454">https://github.com/bbatsov/rubocop/pull/1454</a></div></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>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?<br></div></div></blockquote><div><br></div><div>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]</div><div><br></div><div><div>[2] <a href="https://github.com/bbatsov/ruby-style-guide#one-class-per-file">https://github.com/bbatsov/ruby-style-guide#one-class-per-file</a></div></div><div><br></div><div>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.</div><div><br></div><div>Essentially, we may be able to turn this ...</div><div><br></div><div><font face="monospace">class FlowPage</font></div><div><font face="monospace"> include PageObject</font></div><div><font face="monospace"><br></font></div><div><div><font face="monospace"> # Collapse button</font></div><div><font face="monospace"> a(:topics_and_posts_view, href: "#topics/full")</font></div><div><font face="monospace"> a(:small_topics_view, href: "#topics/compact")</font></div><div><font face="monospace"> a(:topics_only_view, href: "#topics/topics")</font></div><div><font face="monospace"><br></font></div><div><font face="monospace"> # Dialogs</font></div><div><font face="monospace"> div(:dialog, css: ".flow-ui-modal")</font></div><div><font face="monospace"> textarea(:dialog_input, name: "topic_reason")</font></div><div><font face="monospace"> button(:dialog_cancel, css: "a.mw-ui-destructive:nth-child(2)")</font></div><div><font face="monospace"> button(:dialog_submit_delete, text: "Delete")</font></div><div><font face="monospace"> button(:dialog_submit_hide, text: "Hide")</font></div><div><font face="monospace"> button(:dialog_submit_suppress, text: "Suppress")</font></div></div><div><br></div><div><font face="monospace"> # ...</font></div><div><font face="monospace">end</font></div><div><br></div><div>... into something like this ...</div><div><br></div><div><div><font face="monospace">class FlowPage</font></div><div><font face="monospace"> include PageObject</font></div><div><font face="monospace">end</font></div><div><font face="monospace"><br></font></div><div><font face="monospace">module FlowButtons</font></div><div><font face="monospace"> include PageObject</font></div><div><font face="monospace"><br></font></div><div><div><span style="font-family:monospace"> a(:topics_and_posts_view, href: "#topics/full")</span><br></div><div><font face="monospace"> a(:small_topics_view, href: "#topics/compact")</font></div><div><font face="monospace"> a(:topics_only_view, href: "#topics/topics")</font></div><div><font face="monospace">end</font></div><div><font face="monospace"><br></font></div><div><font face="monospace">module FlowDialogs</font></div><div><font face="monospace"> include PageObject</font></div><div><font face="monospace"><br></font></div><div><span style="font-family:monospace"> div(:dialog, css: ".flow-ui-modal")</span><br></div><div><font face="monospace"> textarea(:dialog_input, name: "topic_reason")</font></div><div><font face="monospace"> button(:dialog_cancel, css: "a.mw-ui-destructive:nth-child(2)")</font></div><div><font face="monospace"> button(:dialog_submit_delete, text: "Delete")</font></div><div><font face="monospace"> button(:dialog_submit_hide, text: "Hide")</font></div><div><font face="monospace"> button(:dialog_submit_suppress, text: "Suppress")</font></div></div><div><font face="monospace">end</font></div></div><div><br></div><div>... 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! :)</div></div><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr">Dan Duvall<div>Automation Engineer</div><div><a href="http://wikimediafoundation.org" target="_blank">Wikimedia Foundation</a><br></div></div></div>
</div></div>