Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

christhesoul
Copy link
Contributor

@christhesoul christhesoul commented Sep 21, 2023

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
  • 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.
  • I have added tests to cover my changes.
  • It is safe to rollback this change.

@christhesoul christhesoul requested a review from a team as a code owner September 21, 2023 14:22
@christhesoul christhesoul force-pushed the cw/update-stale-violations-error-message branch from 8e61c2f to 1ac271a Compare September 21, 2023 14:28
@gmcgibbon gmcgibbon merged commit b97f572 into main Sep 21, 2023
21 checks passed
@gmcgibbon gmcgibbon deleted the cw/update-stale-violations-error-message branch September 21, 2023 19:23
@gmcgibbon
Copy link
Member

Thank you!

@christhesoul
Copy link
Contributor Author

@gmcgibbon 🫡

@rafaelfranca
Copy link
Member

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

@rafaelfranca
Copy link
Member

See #138 (comment)

@gmcgibbon
Copy link
Member

gmcgibbon commented Nov 16, 2023

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

@rafaelfranca
Copy link
Member

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

@christhesoul
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

TIL. Thanks for answering all my questions. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants