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

Allow for the specification of the rustfmt path #571

Closed
wants to merge 1 commit into from

Conversation

dfreese
Copy link
Contributor

@dfreese dfreese commented Feb 26, 2021

In some situations, rustfmt can't be counted on to be in your PATH. In
those cases, this adds a format_with(...) function to the builder to
specify the path to rustfmt, which is then passed into the new
format_with function.

A small dance is done here with fmt(...) and format_with(...) to
preserve the public API of the crate, so this, if I understand
correctly, should be an API-compatible change.

I'm not 100% this would be the way I would want to go, but I wanted to
put it up for discussion. There are more roundabout approaches I could
take to format each file in our build system, but I had deferred from
doing that previously. The relevant section of a public draft of the
implementation I had put together is here:

https://github.com/bazelbuild/rules_rust/pull/479/files#r583439508

Also open to suggestions as to where this should be tested. I didn't
see, but may have missed, a good place for it.

@dfreese
Copy link
Contributor Author

dfreese commented Feb 26, 2021

This is an (unintentional) alternative to #566.

In some situations, rustfmt can't be counted on to be in your PATH.  In
those cases, this adds a format_with(...) function to the builder to
specify the path to rustfmt, which is then passed into the new
format_with function.

A small dance is done here with fmt(...) and format_with(...) to
preserve the public API of the crate, so this, if I understand
correctly, should be an API-compatible change.

I'm not 100% this would be the way I would want to go, but I wanted to
put it up for discussion.  There are more roundabout approaches I could
take to format each file in our build system, but I had deferred from
doing that previously.   The relevant section of a public draft of the
implementation I had put together is here:

https://github.com/bazelbuild/rules_rust/pull/479/files#r583439508

Also open to suggestions as to where this should be tested.  I didn't
see, but may have missed, a good place for it.
@davidpdrsn
Copy link
Member

I this is more general than #566 since you can use any path without having to set environment variables?

@dfreese
Copy link
Contributor Author

dfreese commented Feb 26, 2021

I this is more general than #566 since you can use any path without having to set environment variables?

Yeah. I had looked at both when I initially proposed a similar design to prost (https://github.com/danburkert/prost/pull/309). The explicit function to set it in the builder supports environment variables, but avoids their surprises.

Environment variables are a simpler solution, so I get why prost went with that at the time, even though it wouldn't be my first preference.

@davidpdrsn
Copy link
Member

I'll let @LucioFranco make the call here. I'd be fine with either.

@UebelAndre
Copy link
Contributor

UebelAndre commented Feb 28, 2021

I think environment variables are a very common pattern. I'd hope the solution for this issue might match what cargo is doing. I don't see why both couldn't be used.

edit: But my only interest is solving for bazelbuild/rules_rust#479 (comment) so I'm happy to defer to @dfreese

@dfreese
Copy link
Contributor Author

dfreese commented Feb 28, 2021

I think environment variables are a very common pattern. I'd hope the solution for this issue might match what cargo is doing. I don't see why both couldn't be used.

I wasn't aware cargo used that variable to configure cargo fmt, so staying consistent with that would probably make the most sense.

https://doc.rust-lang.org/cargo/reference/environment-variables.html

RUSTFMT — Instead of running rustfmt, cargo fmt will execute this specified rustfmt instance instead.

@LucioFranco
Copy link
Member

@dfreese does #566 work for you or do we still want to add something like this?

@dfreese
Copy link
Contributor Author

dfreese commented Mar 12, 2021

@dfreese does #566 work for you or do we still want to add something like this?

#566 works for me.

@dfreese
Copy link
Contributor Author

dfreese commented Mar 12, 2021

Closing since #566 has been merged.

@dfreese dfreese closed this Mar 12, 2021
@dfreese dfreese deleted the rustfmt branch August 8, 2021 17:29
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.

4 participants