Add "issue-sources" concept to config and make cz-component-dash use it #76

alancutter · 2016-10-14T05:32:28Z

This patch rewrites cz-component-dash to no longer be dependent on Crbug for getting issue data or generating query URLs.
This is a step towards providing GitHub issue data to the same dash using a common Issue JSON format.

This change builds upon #74.
BUG=#72

+@suzyh as author of cz-component-dash.

… Crbug

alancutter · 2016-10-14T05:35:16Z

client/cz-registry.html

-        return this.registeredSources[type].then(source => source.registerQuery(query, callback));
+        return this.registeredSources[type].then(source => {
+          source.registerQuery(query, callback);
+          return source;


Drawing attention to this change.
Is it weird to return a promise to the source element from registerSource()? I added this to support being able to call queryURL() on the issue source element to get an issue vendor specific URL.

This gets used again in #77 where the dash asks the issue source to give its interpretation of a username from the user config as it differs between Crbug and Github.

suzyh

This seems fine to me, but I think Shane should give it a look over. I can't comment on the note about promises.

…IssueList

shans · 2016-10-19T00:51:22Z

Can you bring this up-to-date so it's easier to review what's new?

…ources

alancutter · 2016-10-19T01:50:30Z

Done.

shans

In addition to functionality changes, you've made at least 5 fairly unrelated variable name changes. I guess I have 3 comments:

(1) by changing names and reordering files you've made this significantly harder to review. The functionality changes are obscured by all the incidental changes.

(2) the old names were fine. They really didn't need changing. In many cases you've just taken a different aesthetic stance to the old version, and what's to say that either stance is more correct? You're just making work for both you and your reviewer. Why?

(3) If you absolutely must make name changes unrelated to functionality changes, I suggest you do it as a separate CL in the future, explicitly marked as such, and summarize the changes in the patch header to make it easier for your reviewer. But seriously, don't do this unless there's a good reason.

Having said allll that, I think (but I'm not as confident as I would like to be because of aforementioned obscuring) that this is fine to commit once you change 'breakdown' back to 'component'.

shans · 2016-10-19T02:02:33Z

client/chromez-behaviors.js

@@ -28,17 +29,17 @@ ChromeZBehaviors.AJAXBehavior = {
    }
  },
  handleResponse: function(data) {
-    var query = this.searchQueries[data.model.index];
+    var searchQuery = this.searchQueries[data.model.index];


This rename is super marginal:
(1) you almost don't make use of the parameter at all, so you're adding a lot of noise to the review
(2) there's absolutely nothing wrong with the old name
(3) you don't need to do the rename for your code to work

I'd avoid things like this in the future.

Okay, will change back. I was avoiding query.query.

shans · 2016-10-19T02:03:48Z

client/cz-component-dash.html

@@ -19,7 +19,7 @@
        align-content: flex-start;
      }

-      .component {
+      .breakdown {


again with this one. "breakdown" isn't really more generic than "component", and is more confusing. I'd actually like you to change this back before committing.

shans · 2016-10-19T02:05:47Z

client/cz-component-dash.html

          type: 'Object',
-          value: function() { return {}; },
+          value: function() { return null; },


why this change? Also you don't need a function to initialize objects to null, just use value: null.

I didn't know we could use values directly, cool. I assume the functions are used for non-primitive value types then.

shans · 2016-10-19T02:06:58Z

client/cz-component-dash.html

-          value: function() { return {}; },
+          value: function() { return null; },
+        },
+        issueBreakdown: {


why did you reorder this?

No idea TBH. :/

shans · 2016-10-19T02:07:52Z

client/cz-component-dash.html

      attached: function() {
-        registerSource('cz-config', 'users', function(users) {
+        registerSource('cz-config', 'users', users => {


(this is an example of an unrelated change that I don't mind seeing in a review. It's well contained, improves readability, and doesn't force me to chase changes all over the file.)

shans · 2016-10-19T02:08:25Z

client/cz-component-dash.html

-        }.bind(this));
-        registerSource('cz-config', 'updateSLO', function(updateSLO) {
+        });
+        registerSource('cz-config', 'updateSLO', updateSLO => {
          this.set('updateSLO', updateSLO);


this.updateSLO = updateSLO. Also I think it's fine to put it all on one line now.

I thought we should use this.set() when the property is used in the template otherwise the template is not guaranteed to react to the change. Is this an incorrect assumption?

shans · 2016-10-19T02:12:32Z

client/cz-component-dash.html

-          var teamsIssueList = new IssueList();
-          var othersIssueList = new IssueList();
-          var unownedIssueList = new IssueList();
+      updateIssueSource: function({tag, query}) {


(nit) splitting this out does not improve readability, but does destroy locality of effect

This was a remnant of when I was storing a separate issueSource property that made this split necessary. It is no longer necessary and can be inlined again.

shans · 2016-10-19T02:12:44Z

client/cz-component-dash.html

          for (var issue of issueList) {
            if (issue.owner == null) {
-              unownedIssueList.push(issue);
+              unowned.push(issue);


again: why?

The reasoning here was to make the names team, others and unowned consistent between these local variables and the members of this.component.

shans · 2016-10-19T02:52:43Z

Dude, I don't care what the reasons are :)

Seriously, this is much more about velocity than it is about naming arguments (which are generally bankrupt of logic and full of personal opinion).

shans · 2016-10-19T02:58:15Z

BTW you should be able to merge this yourself now but let me know if that doesn't work.

alancutter · 2016-10-19T03:00:51Z

It's hard not to justify yourself when being asked "why?". (:

Have s/breakdown/component/g'd, will merge. I'm still curious about the this.set() thing though.

alancutter added 2 commits October 14, 2016 16:22

Add issue-sources config and make cz-component-dash use it instead of…

116e860

… Crbug

Fix bugs

ed816b2

alancutter commented Oct 14, 2016

View reviewed changes

alancutter mentioned this pull request Oct 14, 2016

Add cz-github-issues as an issue source for cz-component-dash in animations-ave.json #77

Merged

suzyh reviewed Oct 17, 2016

View reviewed changes

alancutter added 7 commits October 17, 2016 16:59

Merge branch 'refs/heads/returnIssueList' into issueSources

3fada69

Merge branch 'refs/heads/returnIssueList' into issueSources

873c6c6

Merge branch 'refs/heads/returnIssueList' into issueSources

efb5108

Don't return source element

f640260

Merge branch 'refs/heads/returnIssueList' into issueSources

6297f33

Merge remote-tracking branch 'refs/remotes/origin/master' into return…

0aaca8e

…IssueList

Merge branch 'refs/heads/returnIssueList' into issueSources

ea42a22

Merge remote-tracking branch 'refs/remotes/origin/master' into issueS…

db46f33

…ources

shans approved these changes Oct 19, 2016

View reviewed changes

component name

8e0f2e8

alancutter merged commit 8ed366d into shans:master Oct 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "issue-sources" concept to config and make cz-component-dash use it #76

Add "issue-sources" concept to config and make cz-component-dash use it #76

alancutter commented Oct 14, 2016

alancutter Oct 14, 2016

alancutter Oct 17, 2016

suzyh left a comment

shans commented Oct 19, 2016

alancutter commented Oct 19, 2016

shans left a comment

shans Oct 19, 2016

alancutter Oct 19, 2016

shans Oct 19, 2016

shans Oct 19, 2016

alancutter Oct 19, 2016

shans Oct 19, 2016

alancutter Oct 19, 2016

shans Oct 19, 2016

shans Oct 19, 2016

alancutter Oct 19, 2016

shans Oct 19, 2016

alancutter Oct 19, 2016

shans Oct 19, 2016

alancutter Oct 19, 2016

shans commented Oct 19, 2016

shans commented Oct 19, 2016

alancutter commented Oct 19, 2016

Add "issue-sources" concept to config and make cz-component-dash use it #76

Add "issue-sources" concept to config and make cz-component-dash use it #76

Conversation

alancutter commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suzyh left a comment

Choose a reason for hiding this comment

shans commented Oct 19, 2016

alancutter commented Oct 19, 2016

shans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shans commented Oct 19, 2016

shans commented Oct 19, 2016

alancutter commented Oct 19, 2016