ziglint icon indicating copy to clipboard operation
ziglint copied to clipboard

Include `///` and `//!` comments when searching for `TODO`s

Open squeek502 opened this issue 3 years ago • 2 comments

Also trim : if it's found after the TODO (e.g. // TODO: something)


Note: The logic for skipping : is a little verbose to avoid the (admittedly unlikely) situation where it could trim too many colons from something like // TODO ::SomeWeirdThing

squeek502 avatar Jan 10 '22 07:01 squeek502

TODO comments are for the developer and should not be in the documentation

nektro avatar Jan 12 '22 06:01 nektro

I'm fine with maintaining a fork for this feature if you don't want to merge this, but here's some reasoning for why it should be considered:

  • Zig source code has many instances of /// TODO (169 matches across 71 files) and some of //! TODO (7 matches across 3 files)
  • The line between developer and someone reading documentation is fairly blurry, and it's not always fully certain that the target audience of a TODO is only developers

An example from std.ascii.indexOfIgnoreCasePos:

/// Finds `substr` in `container`, ignoring case, starting at `start_index`.
/// TODO boyer-moore algorithm
pub fn indexOfIgnoreCasePos(container: []const u8, start_index: usize, substr: []const u8) ?usize {

I'd argue that seeing this TODO would potentially be useful to a user, as it can give some information about how efficient the function is, and that there are plans for making it more efficient. This could be communicated in a different way while moving the TODO outside of the doc comment, but I think it's not fully clear that that'd be an unambiguous good (and leaving it up to the developer whether or not they want the TODO included in the documentation seems okay to me).


Note that if /// TODO and //! TODO are not included in the TODO linting, then there should probably be a linting rule that specifically warns about their existence (if you want to codify 'TODO comments are for the developer and should not be in the documentation')

squeek502 avatar Jan 12 '22 09:01 squeek502