1220601 - Enable ESLint rule "no-warning-comments" on devtools code

Status: RESOLVED WONTFIX

(DevTools :: General, defect)

Description

[Patrick Brosset <:pbro>]

This is a proposal to enable the no-warning-comments with the following settings:

> "no-warning-comments": [1, { "terms": ["todo", "fixme", "xxx"], "location": "start" }]

This configuration means:
- the rule will only emit warnings
- the rule will emit warnings if it finds the terms (whole words, case insensitive) xxx, fixme or todo at the start of comments.

For example, the following comments would produce warnings:

// XXX: see bug 1219608 to remove this if the count is 1.
// TODO: remove all of this deprecated code: Bug 990137.
// FIXME: Bug 822388 - Use the BreadcrumbsWidget in the Inspector.

The goal is merely to make these comments easier to see, not to prevent entering them in the first place.

Comment 1

[Patrick Brosset <:pbro>]

I'd like to know what other people think.

Comment 2

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

My initial reaction is that I would not want this enabled.

My reasoning is that we may leave such notes in place for a very long time, years in many cases.  I would like it to be possible for files to "lint clean" with no errors or warnings in a shorter time frame than it takes to resolve every message like this.  Since we'll keep adding many such messages anyway, it effectively means we would never hit 0 errors and warnings.

It feels like enabling this entangles passing the linter with "completing every future code change you can think of", and those seem like very different things that should remain separate.  Or it could also discourage faithful linter users from writing helpful comments since it adds a warning, when in fact I believe we **do** want these comments in general.  In my view, the linter is about code style and avoiding common syntax bugs, not for tracking what's left to do in a project, so we should not enable this kind of warning.

Comment 3

[Brian Grinstead [:bgrins]]

Maybe if there's todo/fixme comment *without* a Bug # attached we could get warned, since we shouldn't really have comments like that unless if there's some kind of a path forward to fix it.  But otherwise I agree with Comment 2.

Comment 4

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

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Maybe if there's todo/fixme comment *without* a Bug # attached we could get
> warned, since we shouldn't really have comments like that unless if there's
> some kind of a path forward to fix it.  But otherwise I agree with Comment 2.

Ah, I would agree to that, linting to ensure these messages have a bug number sounds great.  I guess it would need something more complex than the proposed setting, but anyway I like it. :)

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Maybe if there's todo/fixme comment *without* a Bug # attached we could get
> > warned, since we shouldn't really have comments like that unless if there's
> > some kind of a path forward to fix it.  But otherwise I agree with Comment 2.
> 
> Ah, I would agree to that, linting to ensure these messages have a bug
> number sounds great.  I guess it would need something more complex than the
> proposed setting, but anyway I like it. :)

Yeah I don't know if it's possible with eslint.  It would at least need to span multiple one line comments (and/or a multiline comment) and then match 'Bug N' or 'https://bugzilla.mozilla.org/show_bug.cgi?id=N'

Comment 6

[Patrick Brosset <:pbro>]

I agree with comment 2.

In fact, what initially pushed me to file the bug was that these comments indeed *are* useful, and I thought having a tool pick them up would help us make them more useful, not discourage people from writing them.
I thought this could help us see them more easily and not forget about them (I think that these comments, being comments, have a high risk of being forgotten and becoming obsolete at some stage, not in sync with the code around them).
But I see the point that this, unfortunately, mixes them with other types of warnings for things that we *don't* want in the code.
So I agree, let's not enable this rule, let's just keep it in mind for when we want to, say, list all of the TODOs in our codebase (in which case we could just run eslint with a special config with just this rule), anyway I digress.

And I agree with comment 3, this sounds like a very useful rule, and I don't see any technical problem with creating this custom eslint rule. So I'll mark this bug as WONTFIX and file a new one for this rule.