Shopify / packwerk

Good things come in small packages.
MIT License
1.54k stars 111 forks source link

Requalify strict violations #367

Closed alxckn closed 11 months ago

alxckn commented 11 months ago

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?

What should reviewers focus on?

Type of Change

Additional Release Notes

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

Checklist

alxckn commented 11 months ago

It feels like possibly an alternative to https://github.com/Shopify/packwerk/pull/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 commented 11 months ago

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

rafaelfranca commented 11 months ago

CLA needs to be signed before we merge this though.

rafaelfranca commented 11 months ago

Ah, it was.