Update stale violations error message to use bin/packwerk command

Shopify / packwerk

Good things come in small packages.

MIT License

1.59k stars 110 forks source link

Update stale violations error message to use bin/packwerk command #371

Closed christhesoul closed 1 year ago

christhesoul commented 1 year ago

What are you trying to accomplish?

A more correct error message for those of us who just blindly copy and paste things. :)

What approach did you choose and why?

Replaced packwerk with bin/packwerk

What should reviewers focus on?

🤷

Type of Change

[ ] Bugfix
[ ] New feature
[x] 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)

Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.

If no additional notes are necessary, delete this section or leave it unchanged.

Checklist

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

gmcgibbon commented 1 year ago

Thank you!

christhesoul commented 1 year ago

@gmcgibbon 🫡

rafaelfranca commented 11 months ago

This PR is wrong. Reverting. It assumes you have a binstub that you might not have.

rafaelfranca commented 11 months ago

See https://github.com/Shopify/packwerk/pull/138#issuecomment-910753855

gmcgibbon commented 10 months ago

That's true, but shouldn't we generate a binstub in that case to reduce confusion? Right now, the output is misleading.

rafaelfranca commented 10 months ago

It isn't misleading. The documentation assumes people know how bundler works, and it is written with that focus.

christhesoul commented 10 months ago

Sorry to cause problems. 😅

For context, I would often see this in CI and it would tell me, very explicitly, to "run packwerk update-todo", which I would then do inside of a spin shell, and it would fail.

I've worked with Ruby for a bit so I know other things to try, but I don't think I'd claim to have any deep understanding of how bundler works.

(Perhaps naively I think the whole point of bundler is to not have to understand it?)

Anyway, my intention was simply to try and reduce some toil for some devs. If it creates more toil elsewhere then I'm all for the revert.

rafaelfranca commented 10 months ago

bundler actually requires you know now a lot about it. That is why you see a lot of places recommending bundle exec to be used to call commands. Binstubs (the bin/ files) are the equivalent way to tell bundler to execute a command without having to use bundle exec. I don't think we can recommend either of those because some people might not have binstubs, and bundle exec is an anti-patter. The ideal would be for environment to add trusted bin folder to the shell path, so you don't need to remember to use bin/packwerk over packwerk.

christhesoul commented 10 months ago

Why is bundle exec an anti-pattern, out of interest?

I always saw it as a way of saying "I want to run this command within the context of a bunch of dependencies that I have defined in the Gemfile", and over time just used it due to muscle memory.

The ideal would be for environment to add trusted bin folder to the shell path

Yeah, that's a really nice solution. What prevents us doing this in our spin environments?

rafaelfranca commented 10 months ago

What prevents us doing this in our spin environments?

I think nobody got to it. Because we already know a directory is trusted on shadowenv

Why is bundle exec an anti-pattern, out of interest?

Because for some commands, like Rails you activate bundler twice.

christhesoul commented 10 months ago

Because for some commands, like Rails you activate bundler twice.

TIL. Thanks for answering all my questions. ❤️