Closed Bug 1326100 Opened 3 years ago Closed 3 years ago

Allow inline comments in ESLint config

Categories

(DevTools :: General, defect, P3)

49 Branch
defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(2 files)

Our current ESLint rules disallow inline comments (on the same line as code):

```
    // Disallow comments inline after code.
    "no-inline-comments": "error",
```

I think there several cases where these make sense to allow, including:

1. Noting the meaning of boolean args at call sites

```
  certOverrideService.rememberValidityOverride(host, port, cert, overrideBits,
                                               true /* temporary */);
```

2. Explaining a conditional branch

```
if (!Services.appinfo
    || Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT
    || Services.appinfo.ID === undefined /* XPCShell */
    || Services.appinfo.ID == B2G_ID
    || Services.appinfo.ID == GRAPHENE_ID
    || !AddonPathService) {
```

I am sure I can find more if needed...
Tom, Patrick, any opinions?
Flags: needinfo?(ttromey)
Flags: needinfo?(pbrosset)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #1)
> Tom, Patrick, any opinions?

I've also hit some cases where this would have been more clear.
I think it's ok when used judiciously, though of course there's no way
to explain that to eslint.  In the end though I think it's ok
to make this change.  Thanks for bringing this up.
Flags: needinfo?(ttromey)
I have not met cases where this would have helped me personally, and I (personally, again) find examples 1. and 2. sort of hard to read.
But I don't have to deal with code much anymore these days, so I'll go with whatever other people say. Maybe something to bring up at the next meeting, or on the mailing list, and just land if nobody complains?
Flags: needinfo?(pbrosset)
Another form that this rule disallows:

const POPUP_HIDDEN_DELAY = 100; // ms

Currently, the rule would have you rewrite this as:

// ms
const POPUP_HIDDEN_DELAY = 100;

which seems sub-par, since the unit is now further from the number it describes.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Comment on attachment 8849287 [details]
Bug 1326100 - Allow inline comments in DevTools.

https://reviewboard.mozilla.org/r/122104/#review124432

Thank you.
Attachment #8849287 - Flags: review?(ttromey) → review+
Comment on attachment 8849288 [details]
Bug 1326100 - Restore notable inline comments in DevTools.

https://reviewboard.mozilla.org/r/122106/#review124436

Thanks.  You really went the extra mile on this one!  It looks good and in particular I agree that all these comments are better inline.
Attachment #8849288 - Flags: review?(ttromey) → review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0eba13bc9949
Allow inline comments in DevTools. r=tromey
https://hg.mozilla.org/integration/autoland/rev/b0e1eb0bd898
Restore notable inline comments in DevTools. r=tromey
https://hg.mozilla.org/mozilla-central/rev/0eba13bc9949
https://hg.mozilla.org/mozilla-central/rev/b0e1eb0bd898
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.