-
Notifications
You must be signed in to change notification settings - Fork 926
Add options blank_lines_{lower|upper}_bound
to Configurations.md
#2313
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
Conversation
### Example | ||
Original Code: | ||
|
||
```rust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block will break the unmerged test in #2292 because that test expects all Rust code blocks to be preceded by "## `CONFIG_NAME`" and "#### 'CONFIG_VALUE`", in that order. That's the convention that has been followed to this point, at least.
So we need to either update this documentation to match that convention or I need to update the test in #2292 to have a way to opt Rust code blocks out of verification. Is it feasible to modify this documentation or is the example code before the formatted code preferable for clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh, I think that these two configurations are different from the other ones in that changing the value is not "bijective": formatting with a decreased blank line bound will lose information, so increasing the blank line bound and formatting it again won't result in the code we started with. So I wouldn't use the convention here, as it would be less clear to the reader.
I am not sure if there should be a special case in your tests for this; it's probably to complicated right now. So you are probably right: opting out of the test for the first code block and still testing the others is probably fine.
But I'm fine with whatever you (all) decide on :)
Configurations.md
Outdated
|
||
- **Default value**: `1` | ||
- **Possible values**: *unsigned integer* | ||
- **Stable**: Yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you change this to No please? Currently most of the options are unstable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Changed it for both options I added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I left an inline comment.
@LukasKalbertodt Thank you for the update! Would you squash your commits into a single one please? |
@topecongiro I thought GitHub now supports squashing all commits of a PR when merging? But sure, will squash them soon ^_^ |
Closes #2312
I am really not sure if I did everything correctly here. For example: I put the two options at the end of
Configurations.md
, because I didn't know where to put them otherwise. The order ofrustfmt --config-help
doesn't match the order inConfigurations.md
.Just tell me what I could improve. Happy holidays, of course :)