1. Want to run a build from a version from a few months ago, well things are fixed at the head of the main branch, but a todo in the old version are now bad and break the build. No binary searching for where a bug was introduced, or doing a patch of a previous release.
2. Github is down or you don't have internet access, now one of the key strengths of git (distributed, works offline) no longer works.
3. You have a large team, now nobody can get any work done for the length of time it takes a single person to fix the problem.
4. Your automatic cut for the weekly release runs at 3am, but a todo expired at midnight. Now the person in charge of the push to prod needs to do manual work, probably slowing down the cadence of testing the release.
5. Rust has bad compile times, this will only make it worse.
//! # Skipping checks
//!
//! If the environment variable `TODO_OR_DIE_SKIP` is set all macros will do nothing and
//! immediately succeed. This can for example be used to skip checks locally and only perform them
//! on CI.
I think this is a really interesting project, thought-wise. It fills in the other extreme for how important a TODO is. A code comment will certainly be forgotten, but spontaneous build failures are pretty much maximally not-ignorable.
It's almost like an assert for the environment. Maybe in the same vein they should be compiled-out for production builds? (I see in the docs they can be disabled with a flag.) But of course, if builds don't work, then people won't use it or will turn it off, like asserts.
In particular, this highlights just how much I don't think I would want to use this. Especially for environmental changes like a ticket got closed in some 3p project, or a dependency got updated. Probably the main case is the (hopefully rare) times you have to put in a dirty hack, that you _know_ will stop working at a certain date.
Otherwise, the response seems disproportionate. If it's time-intensive to fix and nothing was actually broken, failing the build is a recipe for getting bypassed. Squeaky wheel gets the grease.
Of course, proportionately motivating you to finish medium-ish term projects with questionable immediate value would solve tech debt in general :P
> In particular, this highlights just how much I don't think I would want to use this. Especially for environmental changes like a ticket got closed in some 3p project, or a dependency got updated. Probably the main case is the (hopefully rare) times you have to put in a dirty hack, that you _know_ will stop working at a certain date.
There's nothing that prevents you from commenting out the todo_or_die call that references a GitHub issue and replacing it with a todo_or_die call for next week.
I can see this being useful when you're maintaining multiple versions and forward-porting patches. Some patches only make sense up to a certain version and you don't want to merge them in the next one up.
I do this with warnings, rather than build failures (for the reproducibility issues mentioned). It's especially useful if we need to work around some immediate problem, which will hopefully be fixed in later versions. I don't like depending on external state, so rather than e.g. checking if a github issue is closed, I prefer to either test that the broken functionality is still broken, or else check the version number of the relevant dependency.
Warnings are nice since we can choose how to handle them. Usually it's one of the following cases:
- A warning appears to tell us that some workaround is no longer needed, after we just bumped 'dependency-foo' to a newer version. We can delete that workaround as part of our version bump, reducing our codebase's complexity. (Deleting the workaround has a risk of causing unexpected problems, but so does the update of dependency-foo; in fact, the out-of-date workaround itself is a risk for the update, so deleting it is usually a good idea)
- A warning appears but we're rushing to hit a tight deadline. Things seem to work, so we can take a look later. (This is fine, as long as we actually do the maintenance later ;) )
- A warning appears to tell us that a dependency with known problems has been updated, so our workarounds might not be needed anymore. We try disabling the workaround, but the problem still exists in the new version; we update the check with the new version number, so the warning disappears for now but will re-appear next time that dependency changes.
- A warning appears, but we can ignore it since we're building an old version of our application (e.g. to investigate a regression).
It's a fun idea, of course not practical but a creative one. Something I've done in the past with my team was to agree to never accept a PR with TODO comments that do not have an associated JIRA ticket. So you would need to have:
// TODO (AS-1234): network errors should be handled correctly here
Where AS-1234 is the JIRA id. That way you keep track of them and they are part of your planning discussions. It's simple to do, can potentially be enforced by a linter, and help your product manager understand some of the technical debt.
If you already have a linter, I suggest going a step further and automating the JIRA issue creation when a TODO without an associated issue is found.
Nothing breaks flow quickly than getting stopped by a linter, when an autofixer could be used behind the scenes without ping ponging the developer with linting errors.
> If you already have a linter, I suggest going a step further and automating the JIRA issue creation when a TODO without an associated issue is found.
That's a huge jump in complexity and possibly introducing security issues. Not only does the CI now need to lint your code, it also has to have credentials to your planning tool, and logic for creating issues there when specific things end up in the code. All because a developer couldn't be bothered to add a ticket in the planning tool?
> Nothing breaks flow quickly than getting stopped by a linter, when an autofixer could be used behind the scenes without ping ponging the developer with linting errors.
Linting errors should show up right before commits, and (by default) prevent developers from committing if the code doesn't work (by the standards of linting, checks and tests). This is no different from static type checking then.
If you're waiting for CI runs to see what's wrong with your code, then your feedback cycle is already too slow.
Oh no! A TODO is almost never written with the level of detail I'd want in a JIRA ticket. I think usually, if I'm leaving a few TODOs in a checkin, it's to highlight several locations where a single change needs to be made. Automatically creating new tickets for each call site sounds like hell.
I consider adding TODOs in the code an anti pattern. Those are usually unclear, lack ownership, and purpose of the intended changes gets obsolete. Whenever I read code with TODOs I find it that more often than not they hurt readability. Usually in reviews I ask to resolve or remove them before merge.
Open a bug based on functional (or non functional) impact. Prioritize accordingly. I think if the problem is severe it shouldn't be hidden behind a TODO. If it's not severe then likely it doesn't need attention.
Since these are mostly about things external to a project, I feel like these should be in your CI system but not in the build-time tests. Just like you would do for testing against the latest releases or latest git commits of your dependencies.
Whenever I used TODOs, they are documenting an unwanted behaviour in code that was forced by external circumstances: eg. the version of the OS doesn't support a nice feature. In this case I will add a block beginning with "// TODO(redleader55) - 2021-09-06: remove the workaround and uncomment the code below when OS supports XXXX". You can configure a linter to complain about unowned TODOs, about not having a date or an issue number in the TODO.
There are several differences between a TODO in comments and a compile time check:
1. Such a linter would work for all possible languages with minimal changes - ie. adding the pattern for a comment.
2. You can choose to run the linter only for modified lines.
3. If you need, you can run and gather all TODOs in a project or across the whole codebase from time to time.
4. You can change the requirements - ie. adding an issue/task or the owner to the text of the TODO - without having to update all the old TODOs
5. It doesn't impact the compile time, since it is a comment
It screws up reproducibility--- stuff that built in the past no longer builds when an external condition changes.
I get what this is trying to do, but there's got to be a better mechanism to let your developers know that e.g. a new feature exists upstream than suddenly breaking the build.
Readit News
1. Want to run a build from a version from a few months ago, well things are fixed at the head of the main branch, but a todo in the old version are now bad and break the build. No binary searching for where a bug was introduced, or doing a patch of a previous release.
2. Github is down or you don't have internet access, now one of the key strengths of git (distributed, works offline) no longer works.
3. You have a large team, now nobody can get any work done for the length of time it takes a single person to fix the problem.
4. Your automatic cut for the weekly release runs at 3am, but a todo expired at midnight. Now the person in charge of the push to prod needs to do manual work, probably slowing down the cadence of testing the release.
5. Rust has bad compile times, this will only make it worse.
For 3-4 you can set TODO_OR_DIE_SKIP, as documented for the crate, to disable all the todo checks at compile time.
5 is getting better over time.
Maybe it should emit a warning when the issue is closed and then only error if it has been closed for a year.
Should probably be opt-in, enabled in CI.
Call it cargo todo-or-die or something.
Deleted Comment
It's almost like an assert for the environment. Maybe in the same vein they should be compiled-out for production builds? (I see in the docs they can be disabled with a flag.) But of course, if builds don't work, then people won't use it or will turn it off, like asserts.
In particular, this highlights just how much I don't think I would want to use this. Especially for environmental changes like a ticket got closed in some 3p project, or a dependency got updated. Probably the main case is the (hopefully rare) times you have to put in a dirty hack, that you _know_ will stop working at a certain date.
Otherwise, the response seems disproportionate. If it's time-intensive to fix and nothing was actually broken, failing the build is a recipe for getting bypassed. Squeaky wheel gets the grease.
Of course, proportionately motivating you to finish medium-ish term projects with questionable immediate value would solve tech debt in general :P
There's nothing that prevents you from commenting out the todo_or_die call that references a GitHub issue and replacing it with a todo_or_die call for next week.
Merge request with deleted todos without the fix would not be accepted.
Commenting out is a slight inconvenience, just enough to make sure it gets done.
Warnings are nice since we can choose how to handle them. Usually it's one of the following cases:
- A warning appears to tell us that some workaround is no longer needed, after we just bumped 'dependency-foo' to a newer version. We can delete that workaround as part of our version bump, reducing our codebase's complexity. (Deleting the workaround has a risk of causing unexpected problems, but so does the update of dependency-foo; in fact, the out-of-date workaround itself is a risk for the update, so deleting it is usually a good idea)
- A warning appears but we're rushing to hit a tight deadline. Things seem to work, so we can take a look later. (This is fine, as long as we actually do the maintenance later ;) )
- A warning appears to tell us that a dependency with known problems has been updated, so our workarounds might not be needed anymore. We try disabling the workaround, but the problem still exists in the new version; we update the check with the new version number, so the warning disappears for now but will re-appear next time that dependency changes.
- A warning appears, but we can ignore it since we're building an old version of our application (e.g. to investigate a regression).
// TODO (AS-1234): network errors should be handled correctly here
Where AS-1234 is the JIRA id. That way you keep track of them and they are part of your planning discussions. It's simple to do, can potentially be enforced by a linter, and help your product manager understand some of the technical debt.
Nothing breaks flow quickly than getting stopped by a linter, when an autofixer could be used behind the scenes without ping ponging the developer with linting errors.
That's a huge jump in complexity and possibly introducing security issues. Not only does the CI now need to lint your code, it also has to have credentials to your planning tool, and logic for creating issues there when specific things end up in the code. All because a developer couldn't be bothered to add a ticket in the planning tool?
> Nothing breaks flow quickly than getting stopped by a linter, when an autofixer could be used behind the scenes without ping ponging the developer with linting errors.
Linting errors should show up right before commits, and (by default) prevent developers from committing if the code doesn't work (by the standards of linting, checks and tests). This is no different from static type checking then.
If you're waiting for CI runs to see what's wrong with your code, then your feedback cycle is already too slow.
There are several differences between a TODO in comments and a compile time check:
1. Such a linter would work for all possible languages with minimal changes - ie. adding the pattern for a comment.
2. You can choose to run the linter only for modified lines.
3. If you need, you can run and gather all TODOs in a project or across the whole codebase from time to time.
4. You can change the requirements - ie. adding an issue/task or the owner to the text of the TODO - without having to update all the old TODOs
5. It doesn't impact the compile time, since it is a comment
Though there are still risks it might suddenly hinder you from releasing a build in an emergency.
I get what this is trying to do, but there's got to be a better mechanism to let your developers know that e.g. a new feature exists upstream than suddenly breaking the build.
Although I'd prefer having this throw warnings instead of errors at least locally.