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

add config inline_attribute_width #3409

Merged
merged 3 commits into from
Feb 23, 2019
Merged

Conversation

rchaser53
Copy link
Contributor

@rchaser53 rchaser53 commented Feb 19, 2019

related: #3343

If the line width is width within config width, attribute is inline.
I don't want to change default rustfmt behavior, so config default value is 0.

@rchaser53 rchaser53 changed the title add config inline_attribute_width [WIP]add config inline_attribute_width Feb 20, 2019
Copy link
Contributor

@scampi scampi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! You need to add a section in the documentation about this new option:

---- test::configuration_snippet_tests stdout ----
thread 'test::configuration_snippet_tests' panicked at 'inline_attribute_width does not have a configuration guide', src/test/mod.rs:908:17

tests/source/issue-3343/one.rs Outdated Show resolved Hide resolved
tests/source/issue-3343/two.rs Outdated Show resolved Hide resolved
src/imports.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
src/imports.rs Show resolved Hide resolved
@rchaser53 rchaser53 changed the title [WIP]add config inline_attribute_width add config inline_attribute_width Feb 20, 2019
@rchaser53
Copy link
Contributor Author

Thank you for the review. I fixed it.
I have a problem now.
configuration_snippet_tests is failed, because of version-gate.
What should I do?
Remove version-gate or remove example in Configurations.md?

Copy link
Contributor

@scampi scampi left a comment

Choose a reason for hiding this comment

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

Configurations.md Show resolved Hide resolved
Configurations.md Outdated Show resolved Hide resolved
@topecongiro
Copy link
Contributor

I think that we do not need the version gate in this case. The purpose of the version gate is to ensure that users using the default format style won't be affected by formatting changes between rustfmt version updates.

@topecongiro
Copy link
Contributor

Could you apply this to the attribute on extern crate as well please?

src/imports.rs Outdated Show resolved Hide resolved
tests/target/issue-3343/two.rs Outdated Show resolved Hide resolved
@rchaser53 rchaser53 force-pushed the issue-3343 branch 3 times, most recently from 0e862d8 to c3a168c Compare February 21, 2019 12:23
@rchaser53
Copy link
Contributor Author

Thank a lot!
I reflected your reviews.

Copy link
Contributor

@scampi scampi left a comment

Choose a reason for hiding this comment

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

There is a test failing when run with the beta channel

Mismatch at tests/source/issue-3343.rs:1:
 // rustfmt-config: inline_attribute_width.toml
 
-#[cfg(feature = "alloc")] use core::slice;
+#[cfg(feature = "alloc")]
+use core::slice;
 
-#[cfg(feature = "alloc")] use total_len_is::_50__;
+#[cfg(feature = "alloc")]
+use total_len_is::_50__;
 
 #[cfg(feature = "alloc")]
 use total_len_is::_51___;
Mismatch at tests/source/issue-3343.rs:9:
 
-#[cfg(feature = "alloc")] extern crate len_is_50_;
+#[cfg(feature = "alloc")]
+extern crate len_is_50_;
 
 #[cfg(feature = "alloc")]
 extern crate len_is_51__;

tests/target/issue-3343.rs Outdated Show resolved Hide resolved
tests/source/issue-3343.rs Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
@scampi
Copy link
Contributor

scampi commented Feb 22, 2019

can you add a test to see what happens with @topecongiro comment ? #3409 (comment)

@rchaser53
Copy link
Contributor Author

Thank you for kindness review many times!
I guess this time is OK.

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

@rchaser53 Thank you for your continuous work on this. Other than typos in the documents, this looks good to me.

Configurations.md Outdated Show resolved Hide resolved
src/config/mod.rs Outdated Show resolved Hide resolved
If the line width is width within config width, attribute is inline.
I don't want to change default rustfmt behavior, so config default value is 0.

- fix description
- fix test comment
- remove unnecessary clone
- remove unnecessary test file
- fix test for β version
- attributes => attribute
@rchaser53
Copy link
Contributor Author

Oops. Thank you. I fixed it.

@scampi scampi merged commit 6a75fee into rust-lang:master Feb 23, 2019
@rchaser53 rchaser53 deleted the issue-3343 branch February 23, 2019 12:28
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.

None yet

3 participants