Add expiring-todo-comments rule

[lubien]

Fixes #238

• Due date check.
• Own package version check.
• Dependency package version check.
• Dependency inclusion checks.

I've explored the related issue and picked up some features the seem to the to the liking of those who participated.

If anything let me know so I can work around it.

[lubien]

About the build: apparently no issue is related to my rule and even when I run tests again without my commit it doesn't pass too.

About the change requests. I'll work on that right now!

[sindresorhus]

About the build: apparently no issue is related to my rule and even when I run tests again without my commit it doesn't pass too.

Fixed: 91efcba

[sindresorhus]

@lubien Note: #238 (comment)

If this option is enabled, should it always warn for TODOs without a date / version?

Yes, it would have to replace the functionality of no-warning-comments as it conflicts.

You need to document that the user has to disable the built-in TODO rule.

And this rule needs to handle what the built-in rule does. I think there's a way to import the built-in rule and use that. https://github.com/typescript-eslint/typescript-eslint does that to some rules.

[lubien]

And this rule needs to handle what the built-in rule does. I think there's a way to import the built-in rule and use that. typescript-eslint/typescript-eslint does that to some rules.

I'll look into that

[lubien]

I think there's a way to import the built-in rule and use that.

In a nutshell:

const baseRule = require('eslint/lib/rules/no-warning-comments');
// ...

const rules = baseRule(context);

return {
	Program() {
		rules.Program();
		const comments = sourceCode.getAllComments();
		comments.filter(token => token.type !== 'Shebang').forEach(processComment);
	}
};

But it does make this scenario happen:

  AssertionError [ERR_ASSERTION] (AssertionError) {
    actual: 2,
    code: 'ERR_ASSERTION',
    expected: 1,
    generatedMessage: false,
    operator: 'strictEqual',
    message: `Should have 1 error but had 2: [ { ruleId: 'expiring-todo-comments',␊
        severity: 1,␊
        message: 'Unexpected \\'todo\\' comment.',␊
        line: 1,␊
        column: 1,␊
        nodeType: 'Line',␊
        endLine: 1,␊
        endColumn: 30 },␊
      { ruleId: 'expiring-todo-comments',␊
        severity: 1,␊
        message: 'You have a TODO that past due date 2000-01-01',␊
        line: 1,␊
        column: 1,␊
        nodeType: null,␊
        messageId: 'expiredTodo',␊
        endLine: 1,␊
        endColumn: 30 } ]`,
  }

When I force-run the default no-warn-comments as my rule eslint takes it's errors as mine's.

I'm a little bit lost onto what do next over this information.

[lubien]

If this option is enabled, should it always warn for TODOs without a date / version?

Yes, it would have to replace the functionality of no-warning-comments as it conflicts.

I think I got a better grasp of the situation now. I'll do some tinkering to make the default rule as fallback to mine's.

[lubien]

I might have to setup this repo eslint rule to check for dates if I wanna make this test work. Any recommendation onto how I should do it @sindresorhus ?

Edit: Conversely I can just ignore the expired date test.

[elliot-nelson]

Seems pretty cool for projects that only have a master branch.

For projects with legacy branches (ie you maintain and sometimes release minor versions on older major branches), you likely end up in this scenario where the bug is long since fixed in master but not in other branches that are security-patch-only.

(I don't have a good fix for that, other than not using it in that kind of project I suppose!)

[sindresorhus]

@lubien Can you add a note about the above ⬆️ in the docs? So people are careful if they have that workflow.

[lubien]

@sindresorhus sure. Also I realized I forgot to document about disabling the default rule, will do both.

[yakov116]

Would adding a specific branch name be an option? (Ignore it if it's not the specified branch)
Side note : thank you so much for working on this, it will really improve my work flow!

[lubien]

Would adding a specific branch name be an option? (Ignore it if it's not the specified branch)
Side note : thank you so much for working on this, it will really improve my work flow!

You welcome :D

If @sindresorhus is ok with it I can work on this. Perhaps option disabled by default then you can specify an array of branches for this to work on.

[futpib]

@yakov116 Why not just enable the rule only in the branches you want it to work in?

[yakov116]

@futpib how?

[futpib]

@yakov116 by changing xo or eslint rules option

[yakov116]

@lubien sorry for going off topic. I tried google and reading the docs and I dont see a branch option.

Unless you mean that I should change the config on that branch?

Edit:
If that is the case then it would mean for each branch manually updating the rules.

[futpib]

Yes, that's what I mean. If you want different branches linted differently, in my opinion, they should simply have different linter configs.

Otherwise, imagine if a rule was to work only on master and a branch feature had a violation of that rule that did not show up in CI. Later you merge the feature branch into the master, and the master now does not pass the CI check. (Both branches were "green" before merge, but master became "red" after the merge)

[yakov116]

@futpib Good point

[lubien]

// @sindresorhus

Just did a rebase to fix the package.json conflict.

Sorry for the @ since you're busy but I this has been done for a while and the revisions seems done.

Anything I should go for now?

[yakov116]

@sindresorhus are you able to review this?

[yakov116]

Bump

[sindresorhus]

Sorry for the long delay. I've been on vacation and had limited computer time.

[sindresorhus]

Thanks for all your hard work on this @lubien 🙌 It turned out great 🎉

[lubien]

No problem about delay, I also know you had tons of repos to maintain so I was expecting this.

I'm just glad I had this chance to work with you @ALL