Skip to content

CI-rejected TODO check

I would like to have a thing we can put in our source tree that blocks merges. This is a very useful defence against a wide variety of mistakes (eg, mistakes where a debugging thing is left in).

@nickm and I had an irc conversation (below).

I think my plan is:

  • Write script to reject reject \bXXX+\b (pcre case-sensitive)
  • Fix up the violations in-tree (currently, there are 9) to be some other kind of TODO, or bugfix, or whatever
  • Wire it into CI to run in the last stage as its own job so that you can iterate through CI with an XXX present.
16:48 <+Diziet> nickm: I was wondering if I might persuade to you that 
                something like this would be nice to have in the arti CI:   
https://gitlab.torproject.org/Diziet/rust-derive-adhoc/-/blob/main/maint/check-blocking-todos
16:49 <+Diziet> Then when I put in `DROP THIS COMMIT` I can put an `// XXX` on 
                it too, preventing us from merging it by mistake
16:49 <+Diziet> Err, apparently I chose FIXME in d-a.
16:49 <+nickm> WFM.  But I would prefer /XXXX*/ as the final string.
16:50 <+Diziet> Our existing codebase is a bit full of a mixture of XXX and 
                FIXME and TODO so we might need some discipline or to invent a 
                new tag.
16:50 <+nickm> I tried to use a discipline where XXXX is "this must be fixed. 
               Not to fix this is a bug." and TODO means "maybe someday"
16:50 <+Diziet> nickm: *Four* X's. 
16:50 <+Diziet> (or more)
16:51 <+nickm> XXXX* matches 3 or more
16:51 <+Diziet> discipline> Very happy to adopt your convention, I just want 
                there to be one.
16:51 <+nickm> (I use 4 due to a supersition I got in early teenager years 
               about having my code misidentified as smut)
16:51 <+Diziet> Haha
16:51 <+Diziet> That's why I used FIXME in the d-a script
16:51 <+nickm> (The habit stuck.)
16:52 <+Diziet> OK, I will make a ticket and assign myself.
16:52 <+Diziet> zealot:arti> git-grep XXX | wc -l
16:52 <+Diziet> 10
16:52 <+nickm> one wrinkle is mksteemp
16:52 <+Diziet> Which is doable as a cleanup
16:52 <+nickm> another wrinkle is base64
16:52 <+Diziet> nickm: Yeah, but there's workarounds for that.  Spell it with a 
                circumlocution.
16:53 <+Diziet> base64 can be broken up with a spurious " "
16:53 <+Diziet> FIXME is 5 characters so ~2^10x less likely to come up there...
16:54 <+Diziet> But
16:54 <+Diziet> zealot:arti> git-grep -i '\bFIXME\b' | wc -l
16:54 <+Diziet> 35