1311078 - Use consistent code style for object curly bracket spacing

Status: RESOLVED WORKSFORME

(DevTools :: General, enhancement, P3)

Description

[Tim Nguyen :ntim]

There are places where we use `{ foo: bar }` and some other where we use `{foo: bar}`. We should use a consistent style.
We can enforce this using the object-curly-spacing rule: http://eslint.org/docs/rules/object-curly-spacing

Autofixing (using eslint --fix) is also supported by this rule.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Not sure how we'll choose...  My personal preference is the longer `{ foo: bar }` for ease of reading.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> Not sure how we'll choose...  My personal preference is the longer `{ foo:
> bar }` for ease of reading.

I also share the same preference

Comment 3

[Tim Nguyen :ntim]

Attachment: Bug 1311078 - Use consistent code style for object curly bracket spacing.

Review commit: https://reviewboard.mozilla.org/r/103678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103678/

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8825555 [details]
Bug 1311078 - Use consistent code style for object curly bracket spacing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103678/diff/1-2/

Comment 5

[Tom Tromey :tromey]

After reading through page 7 of the patch (jryans and I agreed to split the review),
I'm thinking that maybe this isn't worth the effort.

I think the change is largely cosmetic.  It's not likely that this rule will prevent a bug.
For me, the resulting code isn't any more clear than the status quo ante (it's not
worse either, it just seems about the same).

So, my inclination would be to say -- let's not bother, instead let's just document that
this is a don't-care option.  However, if others (jryans?  gl?) think this is worth putting
in, I'll proceed with the review.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8825555 [details]
Bug 1311078 - Use consistent code style for object curly bracket spacing.

https://reviewboard.mozilla.org/r/103678/#review104624

So far, I only reviewed pages 1 - 6.

I quite like the change myself, actually.  Yes, it's purely stylistic, but I think it's nice to have consistent approach for these.  For me personally, it removes the mental enengy of thinking "okay, now what style does this file use for braces, since they are all different, but I should probably be internally consistent per file" since there's just one answer for the whole project.  It's on the same level as function spacing or space around if conditions, for example.

I am happy to see there's very few errors in the changes as well.  Nearly all of this looks the way I would write it today.

::: devtools/client/dom/content/components/main-toolbar.js:51
(Diff revision 2)
>    render: function () {
>      return (
>        Toolbar({},
>          ToolbarButton({
>            className: "btn refresh",
> -          onClick: this.onRefresh},
> +          onClick: this.onRefresh },

Fix indentation here

::: devtools/client/framework/toolbox.js:932
(Diff revision 2)
>        return;
>      }
>  
>      // Calculate the height to which the host should be minimized so the
>      // tabbar is still visible.
> -    let toolbarHeight = this._componentMount.getBoxQuads({box: "content"})[0].bounds
> +    let toolbarHeight = this._componentMount.getBoxQuads({ box: "content" })[0].bounds

Maybe we can write this different without `height` hanging there by itself?

::: devtools/client/inspector/rules/models/rule.js:140
(Diff revision 2)
>     *         Promise which resolves with location as an object containing
>     *         both the full and short version of the source string.
>     */
>    getOriginalSourceStrings: function () {
> -    return this.domRule.getOriginalLocation().then(({href,
> -                                                     line, mediaText}) => {
> +    return this.domRule.getOriginalLocation().then(({ href,
> +                                                     line, mediaText }) => {

Fix indent somehow

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8825555 [details]
Bug 1311078 - Use consistent code style for object curly bracket spacing.

https://reviewboard.mozilla.org/r/103678/#review104636

(I meant r+ for myself, didn't realize tromey was already set as reviewer too.)

Comment 8

[Tom Tromey :tromey]

Just FYI, I will finish my part tomorrow.
I saw that it touches reps and I wanted to clarify which repository is canonical.

Comment 9

[Tom Tromey :tromey]

The answer is that m-c is still canonical for the time being.

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8825555 [details]
Bug 1311078 - Use consistent code style for object curly bracket spacing.

https://reviewboard.mozilla.org/r/103678/#review104682

Thank you for doing this.

I am not completely sure I handled this review correctly.  We can discuss it though.

On the one hand, ordinarily I try not to review the context around a patch.  In this patch, though, I've requested a few changes that might be seen that way.  My reasons are:

1. :jryans already did it
2. the lines in question are already being touched by this patch
3. it seemed relevant to the general goals of clarity and consistency

After going through pages 7-12, I also have two other comments.

First, maybe I should have just r+'d this without reading it, on the basis that it was an automated change.  However, that seemed like an abdication of responsibility, so I did read it.

Second, in the future, I would appreciate it if you could split up monster patches like this.  I would have been happier even with a long series, because then I would have felt better about reviewing them individually at a more measured pace.  One way to split would be to convert a directory at a time.

::: devtools/client/jsonview/components/main-tabbed-area.js:54
(Diff revision 2)
>  
>      render: function () {
>        return (
>          Tabs({
>            tabActive: this.state.tabActive,
> -          onAfterChange: this.onTabChanged},
> +          onAfterChange: this.onTabChanged },

The brace here looks misplaced.

::: devtools/client/jsonview/components/main-tabbed-area.js:57
(Diff revision 2)
>          Tabs({
>            tabActive: this.state.tabActive,
> -          onAfterChange: this.onTabChanged},
> +          onAfterChange: this.onTabChanged },
>            TabPanel({
>              className: "json",
> -            title: Locale.$STR("jsonViewer.tab.JSON")},
> +            title: Locale.$STR("jsonViewer.tab.JSON") },

Here too.

::: devtools/client/jsonview/components/main-tabbed-area.js:67
(Diff revision 2)
>                searchFilter: this.state.searchFilter
>              })
>            ),
>            TabPanel({
>              className: "rawdata",
> -            title: Locale.$STR("jsonViewer.tab.RawData")},
> +            title: Locale.$STR("jsonViewer.tab.RawData") },

Here too.

::: devtools/client/jsonview/components/main-tabbed-area.js:75
(Diff revision 2)
>                actions: this.props.actions
>              })
>            ),
>            TabPanel({
>              className: "headers",
> -            title: Locale.$STR("jsonViewer.tab.Headers")},
> +            title: Locale.$STR("jsonViewer.tab.Headers") },

Here too.  I'm marking them all since the patch is so large; normally I'd say "there are others" but I don't want to make you search through it all...

::: devtools/client/shared/components/notification-box.js:209
(Diff revision 2)
>      return (
>        button({
>          key: props.label,
>          className: "notification-button",
>          accesskey: props.accesskey,
> -        onClick: onClick},
> +        onClick: onClick },

This brace seems misplaced.

::: devtools/client/shared/components/notification-box.js:239
(Diff revision 2)
>              )
>            ),
>            div({
>              className: "messageCloseButton",
>              title: this.props.closeButtonTooltip,
> -            onClick: this.close.bind(this, notification)}
> +            onClick: this.close.bind(this, notification) }

Here too.

::: devtools/client/shared/components/reps/array.js:162
(Diff revision 2)
>              className: "arrayRightBracket",
>              object: object
>            }, brackets.right),
>            DOM.span({
>              className: "arrayProperties",
> -            role: "group"}
> +            role: "group" }

Here too.

::: devtools/client/shared/components/reps/grip-array.js:145
(Diff revision 2)
>        let objectLink = this.props.objectLink || span;
>        let title = this.getTitle(object);
>  
>        return (
>          span({
> -          className: "objectBox objectBox-array"},
> +          className: "objectBox objectBox-array" },

Misplaced brace.  I think for these the indentation may have to change as well.

::: devtools/client/shared/components/reps/grip-array.js:158
(Diff revision 2)
>              className: "arrayRightBracket",
>              object: object
>            }, brackets.right),
>            span({
>              className: "arrayProperties",
> -            role: "group"}
> +            role: "group" }

Likewise.

::: devtools/client/shared/components/splitter/split-box.js:193
(Diff revision 2)
>          className: classNames.join(" "),
>          style: style },
>          startPanel ?
>            dom.div({
>              className: endPanelControl ? "uncontrolled" : "controlled",
> -            style: leftPanelStyle},
> +            style: leftPanelStyle },

Here.

::: devtools/client/shared/components/splitter/split-box.js:206
(Diff revision 2)
>            onMove: this.onMove
>          }),
>          endPanel ?
>            dom.div({
>              className: endPanelControl ? "controlled" : "uncontrolled",
> -            style: rightPanelStyle},
> +            style: rightPanelStyle },

Here.

::: devtools/client/shared/components/tabs/tabbar.js:196
(Diff revision 2)
> -      div({className: "devtools-sidebar-tabs"},
> +      div({ className: "devtools-sidebar-tabs" },
>          Tabs({
>            onAllTabsMenuClick: this.onAllTabsMenuClick,
>            showAllTabsMenu: this.props.showAllTabsMenu,
>            tabActive: this.state.activeTab,
> -          onAfterChange: this.onTabChanged},
> +          onAfterChange: this.onTabChanged },

Here.

::: devtools/client/shared/components/tree/label-cell.js:53
(Diff revision 2)
>  
>        return (
>          td({
>            className: "treeLabelCell",
>            key: "default",
> -          style: rowStyle},
> +          style: rowStyle },

Here.

::: devtools/client/shared/components/tree/tree-row.js:165
(Diff revision 2)
>  
>        // Render tree row
>        return (
>          tr({
>            className: classNames.join(" "),
> -          onClick: this.props.onClick},
> +          onClick: this.props.onClick },

Here.

::: devtools/client/shared/components/tree/tree-view.js:326
(Diff revision 2)
>  
>        return (
>          DOM.table({
>            className: classNames.join(" "),
>            cellPadding: 0,
> -          cellSpacing: 0},
> +          cellSpacing: 0 },

Here.

::: devtools/client/webconsole/new-console-output/components/filter-bar.js:144
(Diff revision 2)
>  
>      if (ui.filteredMessageVisible) {
>        children.push(
> -        dom.div({className: "devtools-toolbar"},
> +        dom.div({ className: "devtools-toolbar" },
>            dom.span({
> -            className: "clear"},
> +            className: "clear" },

Another one.

::: devtools/shared/gcli/commands/security.js:270
(Diff revision 2)
>        let referrerUrls = [
>          // add the referrer uri 'referrer' we would send when visiting 'uri'
>          {
>            uri: pageURI.scheme + "://example.com/",
>            referrer: otherDomainReferrer,
> -          description: l10n.lookup("securityReferrerPolicyOtherDomain")},
> +          description: l10n.lookup("securityReferrerPolicyOtherDomain") },

Here

::: devtools/shared/gcli/commands/security.js:274
(Diff revision 2)
>            referrer: otherDomainReferrer,
> -          description: l10n.lookup("securityReferrerPolicyOtherDomain")},
> +          description: l10n.lookup("securityReferrerPolicyOtherDomain") },
>          {
>            uri: sameDomainUri,
>            referrer: sameDomainReferrer,
> -          description: l10n.lookup("securityReferrerPolicySameDomain")}
> +          description: l10n.lookup("securityReferrerPolicySameDomain") }

And here.

Comment 11

[Mark Banner (:standard8)]

object-curly-spacing is now managed by prettier, so there's nothing to do here.