Allow for the specification of the rustfmt path#571
Allow for the specification of the rustfmt path#571dfreese wants to merge 1 commit intohyperium:masterfrom
Conversation
|
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.
|
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. |
|
I'll let @LucioFranco make the call here. I'd be fine with either. |
|
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 |
I wasn't aware cargo used that variable to configure
|
|
Closing since #566 has been merged. |
4 participants
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.