Implemented 'unstable options' command line option #1998
Conversation
tmahmood
changed the title
|
Great, thank you for the PR! Sorry it took me a while to get to it, I'm mostly on parental leave right now and only checking notifications intermittently. |
|
It would be good if you could squash your commits (I find |
src/bin/rustfmt.rs
Outdated
| @@ -122,6 +122,12 @@ fn make_opts() -> Options { | |||
|
|
|||
| opts.optflag( | |||
| "", | |||
| "unstable-features", | |||
| "Enables unstable features. Only available in rust nightlies", | |||
There was a problem hiding this comment.
nit: could you change the text here to end with Only available on the nightly channel please
src/config.rs
Outdated
| #[derive(Clone)] | ||
| pub struct Config { | ||
| // are we using unstable features or not | ||
| unstable_features: bool, |
There was a problem hiding this comment.
Could we use a macro generated option for this like the other options? I think that would simplify things a bit
src/config.rs
Outdated
| } | ||
| )+ | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think we need to be able to set whether each option is stable or unstable, so I don't think we need these getter/setters. A client only needs to be able to set the unstable-features flag itself, which I think it can do with the existing setters.
There was a problem hiding this comment.
I implemented this setter/getter, considering this
(we would need some mechanism for marking options as stable or unstable)"
There was a problem hiding this comment.
That is covered where you added a flag to each option for whether it is stable or unstable.
|
Hello, Sorry about the delayed reply, I was out of town, I've gone through your review and updating my changes accordingly. |
|
Sorry, about the extra commits again. |
|
Most of the changes look good, thanks, but you've removed the 'stable-ness' bool for each option, which I think was a good idea, sorry for the miscommunication there. We should also check when an option is set that if it is unstable, then the unstable features option is set. |
tmahmood
force-pushed
the
master
branch
2 times, most recently
from
b001937 to
7bcd6dd
Compare
|
No problems, I can understand the awesome feelings and the sleepless nights your going through as a parent :) I think I got most of it correctly this time. |
src/config.rs
Outdated
| @@ -254,7 +263,15 @@ macro_rules! create_config { | |||
| impl<'a> ConfigSetter<'a> { | |||
| $( | |||
| pub fn $i(&mut self, value: $ty) { | |||
| (self.0).$i.2 = value; | |||
| // we check if we are using nightly channel | |||
There was a problem hiding this comment.
I don't think we need to check for nightly channel here - if a program is usinf Rustfmt as a library, then it is up to them to enforce the instability invariants.
There was a problem hiding this comment.
However, we should check this when we parse unstable_options from the toml file (there might be macro hygiene problems with this though).
There was a problem hiding this comment.
Haven't faced any problems with macro hygiene hopefully I've understood you correctly and is on the right track.
|
@tmahmood hi, sorry it's taken so long to get back to this. I'm back at work now and should be much quicker to respond. The changes to the unstable features flag looks good. However, it looks like you've pulled in some other commits here (changes to Cargo.*, some other changes to config.rs, etc.). I expect this was a rebasing issue. You can probably use |
tmahmood
force-pushed
the
master
branch
4 times, most recently
from
b3030a2 to
acdf1bb
Compare
|
Good to hear from you. |
|
Awesome, thanks! |
|
@tmahmood if you'd like more rustfmt stuff to work on, then a good follow-up would be to extend the unstable-features flag so that it can apply to command line options and write modes. Each option and write mode could be marked stable or unstable and if unstable, then can only be used if the unstable features flag is set. |
|
Awesome! I would love to continue working. :) |
Hello,
This is my first contribution to any open source project, so any advice would be awesome and thank you for being nice :)
I am checking CFG_RELEASE_CHANNEL environment option to see if we are running on nightly version, I am not confident about this solution. There's must be a better way that I've missed.
All the tests are passing, I've added 2 tests to verify if my changes are working, which are also passing.
Thank you