Repository navigation
Change Flutter TODOs to not require a username, but to require an issue link. #37519
Description
The old syntax was:
// TODO(username): explanation, https://link/to/issue
But the link wasn't required.
The new syntax is:
// TODO: explanation, https://link/to/issue
Where the username is optional, and the link is required.
Activity
gspencergoog commentedon Aug 2, 2019
cc @mklim
mklim commentedon Aug 2, 2019
Also see dart-lang/sdk#57999.
This is going to require multiple steps:
flutter_style_todoslint, or make it more permissive.2 and 3 will need to happen very close to each other or risk failing on new TODOs.
jmagman commentedon Sep 27, 2019
Additional step: update the style guide.
chunhtai commentedon Jun 23, 2021
@gspencergoog Do we still want to make the user name optional?
gspencergoog commentedon Jun 23, 2021
Yes, I think so. I've certainly wanted to add TODOs where I'm not the one with the knowledge of what's going on (and don't know who does), but I have a link to an issue.
chunhtai commentedon Jun 23, 2021
In that case, those todos will be orphans and (likely) no one would ever pick it up.
One of the action item for my todo audit is to make sure every todo has an owner, and Owners will receive email notification periodically to review the current TODO in the repository. The goal is to make sure people are aware of the tech debts they added to the repository.
If you don't have knowledge about the context around the TODO, how do you know there is something need to do?
14 remaining items
Hixie commentedon Mar 27, 2024
I was asked for an update about my opinions on this on Discord, but for completeness let me add my reply here too:
I don't have a strong opinion about whether or not to do this change so long as:
TODO: text, https://urloverTODO(https://url): text)Having the username in the TODO I think has significant psychological value in that it causes most people to think carefully about whether they want to add a TODO or whether they want to just fix the problem in the first place; it also makes it easier to know who to talk to when you trace a bug right to a TODO (which isn't uncommon for well placed TODOs).
Requiring an issue link and a TODO has some additional cost (e.g. increases our issue count artificially, makes it annoying when writing code as you can't really file the issue until the PR lands but also you don't know if the TODO will stay in the PR until it lands, etc; makes it harder for us to easily tell when we can just close an issue, and means we have to keep them in sync -- this gets even worse when there's not a 1:1 mapping).
In general I could also support a world where we just don't add TODOs at all. I often will remove TODOs that have been in the codebase for a while and aren't providing much value. The ones that do provide value could often just be written as normal comments.
I suspect if we require a URL then people will just write TODOs without the word "TODO" when they want to leave one without the overhead of creating an issue.
gspencergoog commentedon Mar 28, 2024
I agree with all four of Ian's "so long as" points above.
For my thought process: I was thinking that the reluctance that people might have to putting their name by it might be causing us to not be aware of some of the technical debt in our system because people just don't add a TODO because they feel intimidated by adding their name to it explicitly (Of course, they're also already associated with a comment line in the code via the blame list, it's just slightly harder to see).
I do see Ian's point about requiring an issue being a stumbling block along the same lines: you might not put in a TODO at all if it's annoying to create the issue (and it often is). It just seems like an issue has the right attributes that I was looking for: something where we can put more detail and discuss, that can be updated over time, and reassigned and given priorities.
Making adding a TODO psychologically easier to do seems like a good idea to me, as long as there's a chance we can understand more details about it someday to fix it and/or remove it. I mean, unless the goal is just to keep the number of TODOs down to make our tech debt numbers look good. :-)
We could just settle for recommending adding an issue, but not requiring it, I guess. But then there's precious little information in a TODO other than the description of the PR that added it (and that's dated, and probably won't be updated).
We could just not add TODOs at all and instead make the policy "If you were going to add a TODO, if you think it's important, just file an issue with the tech-debt label instead" and then we track it from then on as an issue. It makes it harder to know to remove it, though, if it's not linked to the pertinent code, although the location of a TODO already often ends up not having much to do with the location of its solution.
So, I guess my preference is still that we require links but not names, because that seems to me like it has the most value in the long term. If that's unpopular, then I think I'd just choose to get rid of TODOs and recommend that people add implementation comments and file issues instead (perhaps referenced from the comment if it seems helpful).
matanlurey commentedon Mar 28, 2024
Sounds good. I'm going to chat with the analyzer time on how to best contribute a change now that the linter lives in
dart-lang/sdk. My goal is to make this as painless as possible, so likely something like:flutter/flutter)Hopefully the time between "allow" and "disallow" is relatively low (measured in ~weeks not ~quarters).
bwilkerson commentedon Mar 28, 2024
I'm agnostic concerning the change to the Flutter team's style of TODO comments, but requiring a URL would be a breaking change for any users outside the Flutter team that have also enabled the lint (a non-zero number). When such a user downloads the next version built after this change fully lands they will find every TODO in their code base being flagged by the lint. And unfortunately, I can't think of any way to automate the fixes.
Please consider the possibility of making this a new lint rule and switching Flutter from the old to the new. Introducing a new lint wouldn't be a breaking change.
matanlurey commentedon Mar 28, 2024
But this is our own lint, if we can't introduce changes to our own lint then what was the point?
I could imagine writing
flutter_style_todos_2(probably better named asprefer_todos_with_links) that temporarily allows both styles (i.e. the same story as above, just with a different line name). Is that better? I want to avoid having to atomically change all of the TODOs at once.Hixie commentedon Mar 28, 2024
Please don't have a mode where both styles are allowed, that is a recipe for us to end up in a mess. We've done this kind of thing before and universally regretted it.
I am happy to be the one to land the TODO changes atomically if you're just worried about the pain that involves.
matanlurey commentedon Mar 28, 2024
That's sort of the industry standard for making these kinds of changes, but I won't decline someone else doing work for me.
bwilkerson commentedon Mar 28, 2024
No, it isn't our own lint. Yes, it's a lint added to enforce the style of TODOs the Flutter team was using at the time, but it was published as a lint available to everyone in the Dart and Flutter community, and some of them are using it.
If you want the ability to have a set of lints used by the Flutter team that nobody else can enable for their code, then we'll need to talk about how to make that possible. At the moment isn't an option.
matanlurey commentedon Mar 28, 2024
Gotcha, my mistake then
natebosch commentedon Mar 29, 2024
Internal style recommends
// TODO: $LINK - $DETAILS. If we are migrating to a new style I think aiming for consistency is the best default choice. One small difference though is that internal links tend to be short. The length of a full github URL could be a good reason to push the link to the end of the line.The internal guide also prefers issue links to usernames. I do not think adding a username to a TODO is likely to be a meaningful incentive for good engineering practices. Issue links are more useful to a future reader than a
git blameshortcut. A thoughtful TODO description is more useful than a username and vague description.I think it would be OK either way to allow or disallow TODOs with no links. If we allow a TODO with no link a reviewer should hopefully check that the TODO has both a clear description of the anticipated change and when we should make that change. The style guide should focus more on whether a TODO is clearly actionable than whether it repeats
git blameinfo.matanlurey commentedon Mar 29, 2024
It doesn't sound like we have consensus over the solution then.
I also agree with Nate that it's unlikely we could fit most URLs on one line, meaning the lint would have to permissive for overflows, i.e.:
At that point, not only does this lint become hard to write (how many lines, 2? what about 3?), but it sounds like we're likely to diverge and just create another lint style that nobody is really happy with.
My suggestion would be to just drop the lint we have now and have no lint, but I think others would strongly push back, and I don't have the bandwidth to continue to push, so I'll keep this issue open for the next person.