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

Add a cargo feature to disable rustfmt #1614

Closed
lopopolo opened this issue Sep 5, 2019 · 3 comments · Fixed by #1615
Closed

Add a cargo feature to disable rustfmt #1614

lopopolo opened this issue Sep 5, 2019 · 3 comments · Fixed by #1615

Comments

@lopopolo
Copy link
Contributor

lopopolo commented Sep 5, 2019

I never rustfmt generated bindings and I do not check them into source control.

rustfmt'ing brings in a dependency on which -> failure -> backtrace that I otherwise do not need.

Can we add a cargo feature to disable rustfmting of code and make the which crate an optional dependency?

@emilio
Copy link
Contributor

emilio commented Sep 5, 2019

Yeah, that seems pretty reasonable. Given rustfmt'ing is a best-effort thing anyway it may be worth not touching the public API and just early-return from the rustfmt path.

It may be worth rather than a feature to disabling rustfmt, adding a default-feature enable auto-finding rustfmt (and thus which), but still allow to run rustfmt via the env variable or what explicit option if appropriate, that is, basically, adding a feature called which or something, then cfg-ing this match out based on that and returning an error directly:

match which::which("rustfmt") {

That allows to run rustfmt if explicitly asked for it and skip the which dependency. Are you ok with this @lopopolo? Would you be willing to send a patch for that?

@lopopolo
Copy link
Contributor Author

lopopolo commented Sep 5, 2019

@emilio I’d be happy to pick this up. I am headed out on vacation tomorrow though so it may be a couple of weeks.

@lopopolo
Copy link
Contributor Author

lopopolo commented Sep 5, 2019

I had some time today and this was easier than I expected so I got the PR put together 😄

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

Successfully merging a pull request may close this issue.

2 participants