Rule proposal: Expiring TODO comments #238
Comments
|
Should they be warnings before the date/version and errors after, or not reported before and reported as warnings after? Is it ok for a random contributor to get a TODO he has no idea about reported, just because he happen to contribute at the time of it's expiry? |
|
I really like the "from version 2" variant of this feature because it can give you a way to start progressively cleaning up TODOs. If this option is enabled, should it always warn for TODOs without a date / version? |
Silent before and error/warning (depends on what the user sets in their config) after.
Hmm, good point. We could have an opt-in option to not report in pull requests? Can be detected with https://github.com/watson/ci-info#ciispr
Yes, it would have to replace the functionality of |
|
Anything else we cool could do with TODO comments other than versions and dates? |
I've seen specific tools for making sure each TODO has an issue / bug number which is really helpful for making sure TODOs are actually being tracked. It would require an opinion on how to format it. Perhaps, The two common ways I see bug tickets formatted are In terms of dates, treating the date like a timestamp of when the TODO was added might be more intuitive. If I saw: I would probably think, "This TODO was added on 05-01". Six months later when the linter complains that I have a six month old TODO I could either:
|
|
The other thing one could optionally require in a TODO is an identifier for the author, like a name, username/handle, initials, or email address. I know using the blame/annotate features of your VCS can usually tell you who wrote a line of code, there are situations where that history is lost:
In all those cases knowing who originally wrote the comment is helpful. |
|
There are also some other commonly-used comments like |
I could see this being useful for companies. Personally, I prefer just keeping my TODOs only in code. Instead having named formats, we could just accept a regex.
That's a different use-case than what I proposed though. This limits the duration of a TODO, while mine is for a fixed date. Both are valid, but they serve different purposes. What if we made the syntax for my proposal |
👍 The common syntax I have seen for this is |
👍 |
|
Continuing the idea about semver, should be doable to require a version of a dependency? E.g., // TODO [react@>=16.8]: refactor to hooks |
|
@alexzherdev Good idea! That would indeed be very useful. |
|
Also consider TODOs that pop up when a dependency is removed or added (in case you are migrating between substitute packages like Not sure which syntax will be best: // TODO [-lodash]: A workaround for lodash#12345
// TODO [+react]: After switching to react, we will not need this hackor // TODO [npm r lodash]: A workaround for lodash#12345
// TODO [npm i react]: After switching to react, we will not need this hack |
|
|
|
Conditions based on the // TODO [engines node>=10]: Use named capture groupsInspired by this comment #244 (comment) |
|
@futpib Yes, that would be very useful for me. Add a lot of those comments so I will remember to improve the code in the future. |
|
@IssueHunt has funded $100.00 to this issue.
|
|
I wonder if dates should be treated as UTC rather than local to prevent a possibility of an error reported in CI but not locally (or similarly for collaborators in different timezones). Or maybe we should require timezone explicitly in the date format. |
|
Yes, I think we should treat them as UTC. |
|
Would love to work on this rule proposal if it is not already assigned! :) |
issuehunt-oss
bot
added
the
💵 Funded on Issuehunt
Fixes sindresorhus#238 * Due date check. * Own package version check. * Dependency package version check. * Dependency inclusion checks.
Fixes sindresorhus#238 * Due date check. * Own package version check. * Dependency package version check. * Dependency inclusion checks.
|
@lubien Any thoughts on #238 (comment) ? |
So far I enjoyed implementing this rule (never had experience with making my own eslint rule before) so it was a nice opportunity specially since it was by far the most out of the box eslint rule I've heard about and sure picked my interest at first sight. Some points that still puzzle me at some points are:
|
Fixes sindresorhus#238 * Due date check. * Own package version check. * Dependency package version check. * Dependency inclusion checks.
Oh. I'll add to my TODO list. No pun intended. |
|
Yup, pretty much finished it. |
The built-in no-warning-comments rule defaults to |
Fixes sindresorhus#238 * Due date check. * Own package version check. * Dependency package version check. * Dependency inclusion checks.
|
Sorry to join the party a bit late, but I have the following suggested improvements and customizations. Some could be implemented over time, but I think it is helpful to have in mind some of what I see as some very cool additional possibilities here... Some proposed to-dos for the to-dos
Edit: One other item: intended version number release for querying (useful so that to-dos can be built in a hierarchy (even in one's Markdown) by functionality or concept rather than priority) |
|
1: Maybe there should be a rename but just 2: Could you elaborate? 3 and 4(i): Great but how this relate to the failing/warning aspect of eslint? Legit question. 4(ii), 5 and 7: Would be a lot useful to have this kind of report especially when you're hunting todos! I feel this binary is pretty doable for sure but before I try some shady dealing do you happen to know any case like this? (Specially for ESLint to get some implementation ideas but also in another languages, perhaps). 6: Good point. 8: I forgot that one. Got some examples that relate to our discussion that I could learn from? My ideals apart. @sindresorhus do you feel we should go for this approach or for this kind of rule we should make the scope simple as is? |
Updated and added a question. |
Todos in comments are not a particularly great way to organize things -- if someone needs things like priorities than they should move the todos to an issue tracker and reference the issue number in the todo.
All of the possible intent you discuss should be "encoded" in the expiration syntax by the person writing the todo. The warning check should stay simple enough to provide an unambiguous warning. Taking a big step back: The point of a lint rule is to enforce code style and standards. Todos are generally bad for code health. The premise of this rule is to allow todos for a short period of time with a trigger for when they should be removed. Expanding beyond that purpose sounds too complicated and too much of an opinion farm around what configurations should or should not exist.
This is what issue trackers are for. I would heavily discourage trying to maintain this kind of information in the code base itself and definitely not via a lint rule.
A |
|
1: Up to y'all, but to me, some of the most powerful packages are short and sweet in their names, implying that they are meant to "do it all" more or less. 2: If the rule def. were 3 and 4(i): The scopes and priorities would relate to failing/warning because the following rule would only cause priorities "High" (or "1") to fail (and be optionally fixable): Scopes would have similar uses, both to querying/formatters and even during regular code linting, e.g., one might wish to ignore 4(ii), 5 and 7: Which aspect of 4ii were you not clear on? Re: no. 5, the binary could perhaps just be a wrapper around the eslint command line (documented at https://eslint.org/docs/user-guide/command-line-interface ), e.g., using Node's exec or spawn to run the eslint command perhaps. Or it could directly use the Node API, e.g., with https://eslint.org/docs/developer-guide/nodejs-api#cliengine . You might find some command-line argument parsing tools helpful like https://www.npmjs.com/package/command-line-args . Re: no. 7, see https://eslint.org/docs/developer-guide/working-with-custom-formatters (I'm not clear if it receives the rule options on the 8: /**
* @todo Some long,
* multiline to-do
* comment.
*/One could use https://github.com/yavorskiy/comment-parser/ to get the todo collapsed into "Some long, multiline to-do comment". |
|
2: Sounds great. 3 and 4(i): I'm inclined to say now I see, as @MrHen said, it's probably better go for a issue tracker. 4(ii), 5 and 7: It was clear and I am also used to NodeJS CLIs, thanks. I just wanted to know if you had seen some sort of integration directly with ESLint like this scenario or maybe in another programming language (because I feel like there must be something like this). 8: I see. Worth checking. |
I disagree. I use them all of the time and don't find it redundant with trackers or conducive to disorganization, and would use them even more if I didn't have to find-in-files. I use issue trackers for items of public interest, or where a lot more explanation is justified or for which I feel the time is justified to add them there. I keep track of other to-dos, particularly refactoring ones, inline along with the context. There's always a trade-off between whether to store the to-dos inline or in the tracker (unless the tracker software could maintain links and direct editing in context). There's also a spectrum of how much detail one would wish to put inline and what would necessitate a tracker issue. But that doesn't mean one must be dogmatic and refuse to put anything inline around the context where it needs to get implemented and where it is likely to be discovered by someone digging into the code. And if one can query one's inline rules and get them into a formatted report, it can include some of the best of both approaches--having context, but seeing a comprehensive list.
I also disagree here. Sometimes I am interested in my low priority to-dos, and sometimes in my high priority ones. Or those of a particular scope. People who want a simple warning without the to-do text appended or replacing the message can do so.
The purpose of a lint rule is whatever one finds useful for them. I find it useful to see and query the messages of my to-dos alongside coding errors and styling problems because, as you say, to-dos can be bad for code health, but it would help to have control over which types were reported, just as one has control over whether to report/fix stylistic suggestions or errors, since one does not have all the time in the world to improve everything that needs improving all at one time.
To-dos indeed can be bad for code health, and as mentioned, it would help if the developer had control over the types reported. But to-dos can also be good for code health, as a reminder to refactor later when one has more time is better than having no reminder or putting it in a tracker where it might get forgotten being outside of the original context, or it might deter the developer from fixing because after looking at the tracker, they have to dig through the code to find where that code in need of refactoring is. Whereas if they run a query, they can jump to that line quickly without need of duplicating links and line numbers within an issue tracker.
Given the customization options mentioned, a project can determine whatever configuraiton it wishes to enforce. Defaults can be provided so there is no need for the work of customization unless desired.
I disagree that it has no place in a linting rule, as some to-dos are helpful to be discoverable during the linting process. Whether it has a place in this project is not my call. |
I think the current scope is suitable for the first pass on this rule. My recommendation is to focus on the current PR by @lubien. That work would be necessary to support what you are proposing anyway. Once that PR is in, this issue should be closed. If you want to continue championing these ideas, let's split them off into a different issue so we can discuss them in more detail without conflating them with the scope in @lubien's PR. |
|
Do people consider it acceptable for items 2, 6, 8 to be in scope though? i.e.
no. 8 would imo be a "nice to have" and not too far out of scope for this PR, I think, but 2 and 6 I feel to be very important. |
Out-of-scope, as I'm not yet convinced this is a good idea.
Out-of-scope, as I'm not yet convinced this is a good idea.
I think we can at least split jsdoc support off to a separate PR. The PR is big enough as it is and starting to stabilize. I don't think it should be a requirement for the existing bounty (but the final call on that is for @sindresorhus.) |
To be honest, I think you want a very different direction for this rule than we do, and it would probably be easier for you to create your own rule. |
|
Yup, does sounds like that. Though I do like the features you are adding, I'd like a lot more. So carry on, I'll leave you to it! :) |
Fixes sindresorhus#238 * Due date check. * Own package version check. * Dependency package version check. * Dependency inclusion checks.
|
@sindresorhus has rewarded $90.00 to @lubien. See it on IssueHunt
|
issuehunt-oss
bot
added
🎁 Rewarded on Issuehunt
For example,
// TODO [2018-05-01]: Do this thingwould be reported from that date and onwards. The date format should beYYYY-MM-DD(ISO 8601).Would also be nice to support semver, for example,
// TODO [>=2]: Do this thing, which would make it report from version 2 and later.IssueHunt Summary
Backers (Total: $100.00)
Submitted pull Requests
expiring-todo-commentsrule (#238)Tips
IssueHunt has been backed by the following sponsors. Become a sponsor
The text was updated successfully, but these errors were encountered: