Skip to content

Suppress unstable config options by default. #2612

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

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

peterhuene
Copy link
Contributor

This PR suppresses the output of unstable config options by default.

Users can specify the --unstable-features option to show the config options
that are unstable.

Fixes #2611.


let s = str::from_utf8(&output).unwrap();

assert_eq!(s.contains("newline_style"), true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how brittle this makes the test with respect to the options defined in the create_config! macro above. I'd love some guidance on the best way to test this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make another usage of create_config in a different module with mock data and test using that, rather than the real one - Config is the only name you can use with that macro, but if you use it in a different module, then it is a different name. You could do this within the test module and then only refer to it using the qualified path, e.g., something like:

mod mock_config {
    create_config! { ... }
}

#[test]
fn test_...() {
    mock_config::Config::print_docs(...);
}

@peterhuene peterhuene force-pushed the suppress-unstable-config-options branch from a8a3a01 to f470541 Compare April 9, 2018 07:56
Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you, this looks great! I left a comment about testing, but if that doesn't work out, then I think the test you've written are fine - many tests are much more fragile wrt the config options than this.

It would be good if when printing the docs, if an option is unstable then it is marked as such, e.g., with (unstable) after the option name or something similar.


let s = str::from_utf8(&output).unwrap();

assert_eq!(s.contains("newline_style"), true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make another usage of create_config in a different module with mock data and test using that, rather than the real one - Config is the only name you can use with that macro, but if you use it in a different module, then it is a different name. You could do this within the test module and then only refer to it using the qualified path, e.g., something like:

mod mock_config {
    create_config! { ... }
}

#[test]
fn test_...() {
    mock_config::Config::print_docs(...);
}

@peterhuene
Copy link
Contributor Author

Thanks for the feedback! I'll amend the commit to include the (unstable) modifier in the output and update the tests to use their own macro invocation.

This commit suppresses the output of unstable config options by default.

Users can specify the `--unstable-features` option to show the config options
that are unstable.

Fixes rust-lang#2611.
@peterhuene peterhuene force-pushed the suppress-unstable-config-options branch from f470541 to 8208f8a Compare April 9, 2018 19:47
@peterhuene
Copy link
Contributor Author

@nrc I've pushed up adding (unstable) to the option output as well as using a separate mock create_options! invocation. Unfortunately the macro call needed several of the actual options because the generated functions have a dependency on them.

Let me know what you think; I'm happy to fix anything that needs fixing.

@nrc nrc merged commit 518ece2 into rust-lang:master Apr 10, 2018
@nrc
Copy link
Member

nrc commented Apr 10, 2018

This looks great thanks! I'm glad the mock module worked out.

If you'd like something else to work in Rustfmt, then adding support for unstable command line flags might be interesting - it is fairly well related to this work, but a little bit more involved. See #1976 (comment) (and the rest of that issue) for details, happy to answer any questions if that sounds interesting.

@peterhuene peterhuene deleted the suppress-unstable-config-options branch April 10, 2018 18:19
@peterhuene
Copy link
Contributor Author

I'd love to help out. I may have some time this week in the evenings or this coming weekend to look into that issue and see how I can help. Thanks for the opportunity to contribute!

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.

2 participants