This message was imported from the Ruby/Rails Modularity Slack server. Find more info in the import thread.
Is there a check mode like strict but not that strict? strict won’t let me add new todos, but it won’t allow existing todos either. true allows existing todos but it’ll hapily allow me to add new todos. I want a mmode that allows me to have existing todos, and resolve them over time, but that won’t allow me to add new todos. Does that mode exist?
Message originally sent by slack user U70TIGAX94P
IMO added todos should be rejected by humans in code reviews, unless there is a good reason. You could also define CODEOWNERS for the todo files to facilitate that.
AFAIK packwerk doesn’t have multiple strict modes, and I am not even myself sure a strict mode makes sense at all.
Plus, if you have existing violations, it is likely that they move around, which would be indistinguishable from removing one and adding one.
I think this is just a case where the human touch works better.
There’s actually an open PR in the shopify packwerk github repo that does just this. We’re thinking of changing the behavior of strict mode to allow some violations, but no new ones, which would make it more useful (compared to what it can do right now, which is only that it can be turned on when all violations are gone). Your input/expectations/use cases in that PR would be really helpful!
Philip I agree with you in theory that we should simply not be adding new violations and they should be flagged. I’ve found there are a lot of things people want to express with the tool. For example, “I enforce X but if a new violation needs to be added, no big deal, because in part we’re just using this to better understand ongoing use cases” or “I enforce X and we should have no new violations, and we believe 100% of new code should use public API/explicitly manage dependencies!” You could definitely argue that one can (or even – must) manage these concerns via client behavior and culture,
Message originally sent by slack user U70TIGAX94P
Mmmh, I should check out that PR. Not sure how you would distinguish between addition and modification.
You can’t easily (renames/moves are detected in danger-packwerk, but only because it has access to the git context, which we wouldn’t want for packwerk). If the change were implemented, a downside would definitely be false positives on renames of files that have violations.
This is great. Glad to hear there’s a PR for this already.
I’ve just posted my thoughts as a comment in the PR itself: https://github.com/Shopify/packwerk/pull/368#issuecomment-1711635841
TL;DR: I think if we add a new stop_the_bleeding mode which focuses on avoiding to increase the count of violations (regardless of the specific violations), then that’d be ideal because it solves the problem of moving files around.
Message originally sent by slack user U70TIGAX94P
As a datapoint, Packwerk has been used to “stop the bleeding” at Shopify since its inception. That was one of the main reasons to develop it. I still don’t think a separate mode is needed.
Message originally sent by slack user U70TIGAX94P
The packages that still had growing todo lists were either
• affected by a design problem that was out of scope for that package’s stewards to solve (e.g. new data fetching patterns for GraphQL) or
• owned by teams that didn’t care about modularization, which is a problem that can not be solved by tooling.
I can totally buy that argument. For the first, Gusto uses ignored_dependencies and ignored_private_constants to allow the todo list to continue to represent “things we really care about.” And to your point, if the user doesn’t actually care about other items in the todo list, why not just turn off the enforcement entirely?