Requalify strict violations

[alxckn]

What are you trying to accomplish?

This is a proposal to change how strict_mode_violations offenses are gathered and prevent the update_todo from updating a package_todo.yml file with new violations once the strict mode is activated for a package.

Currently the OffenseCollection class will only check whether an offense qualifies as a strict mode violation if it was already present in the file package_todo.yml, I think it should be up to the checker to determine whether offenses, known or unknown, qualify as strict mode violations.

Down the line, the goal would be to enable checkers to apply a 'less strict' mode where new violations cannot be added to the package_todo.yml file but violations that were written to the file before do not cause an error. This would be for packages where it is difficult to reach an empty package_todo.yml but we want to stop the bleeding.

What approach did you choose and why?

• Always ask the checker whether an offense is a strict mode violation and add it to the list if it is
• Update of package_todo.yml does not include new strict mode violations. Being able to update the list with strict mode violations may lead to errors where new violations are not considered new after the file is updated

What should reviewers focus on?

• Current usage of the gem and whether it would harm some existing workflows (I am not sure it would as a zero strict mode violations policy is currently enforced by all checkers)

Type of Change

• Bugfix
• New feature
• Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

• Breaking change (fix or feature that would cause existing functionality to change)

Update with the update_todo command of package_todo.yml would fail in cases where it would work today

Checklist

• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• It is safe to rollback this change.

[alexevanczuk]

I really like the idea here. The fact that strict mode will only fail after you run update and then run check makes it a lot less useful, because most folks will run update and assume everything is fine without running check, and then wait until CI fails to realize they are violating strict mode.

Failing during update makes a lot of sense to me, since it reduces the number and duration of feedback loops. I find the idea of a softer strict mode interesting. Allowing the violations to exist in the TODO file, but not permit new ones to be added with the tooling is definitely interesting. Technically this can be bypassed by updating TODO files by hand, but I think that's okay (since we're trying to create happy-path guard rails, rather than a 'barbed wire fence').

It feels like possibly an alternative to #365, cc @athieriot

[alexevanczuk]

It looks like if there are any strict mode violations, we fail out immediately and do not make any changes to TODO files.

What do you think of instead making all the changes we can make when updating, but then if there are any strict mode violations, we inform the user and exit with a failure?

[alxckn]

I think your proposal makes more sense. I reworked the PR in ef4d6c6.
OffenseCollection does not update the package-todo for strict mode violations that it doesn't already know about (we need to add the entries already known to avoid stale violation errors).
This way, we can run the update_todo command and it won't fail early, it will update what it can, it won't add entries that qualify as strict violation. And as suggested, the strict violation offenses are displayed and the exit code indicates a failure.

I can update the PR description tomorrow if that works for you?

[alxckn]

It feels like possibly an alternative to #365, cc @athieriot

I think what this PR proposes and what's in @athieriot's PR could be complementary.
At Doctolib we have some heterogeneous needs with packages where we'd like to use strict mode only for new violations (this PR + changes in the way strict mode violations are computed at the checker level) and other packages where we'd like to enforce strict mode for 'application' code but we can't enforce it in test setups.

[alexevanczuk]

I like this cleanup – already_listed? was returning a boolean but also mutating data. Now it's two separate operations which is a lot more clean!

[alexevanczuk]

It feels like this message should say something about not being able to successfully add certain violations to the TODO list due to strict mode. Right now it's unclear why it wasn't added.

[alxckn]

You're right, I've amended the message to only include strict violations that are blocking for the update_todo command to complete successfully and changed the final message to explain that something went wrong.

[alexevanczuk]

This looks good to me. @rafaelfranca do you have any feedback or are we good to merge and release?

[rafaelfranca]

CLA needs to be signed before we merge this though.

[rafaelfranca]

Ah, it was.