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

Try to solve issue 3417. #3535

Merged
merged 7 commits into from
May 16, 2019
Merged

Try to solve issue 3417. #3535

merged 7 commits into from
May 16, 2019

Conversation

xiongmao86
Copy link
Contributor

@xiongmao86 xiongmao86 commented May 1, 2019

Add two tests first for #3417. Close #3417.

src/comment.rs Outdated
match crate::format_code_block(&self.code_block_buffer, &config) {
Some(ref s) => trim_custom_comment_prefix(&s.snippet),
None => trim_custom_comment_prefix(&self.code_block_buffer),
if config.format_doc_comments() {
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 know how to write this if block a better way, please give me suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I wonder how to select multiply lines in the commit view. please give me suggestion.

@xiongmao86
Copy link
Contributor Author

Setting normalize_comments and wrap_comments to true no longer imply setting format_doc_comments. And I wonder should I change the option format_doc_comments to format_code_in_doc_comments or just change the description of the option?

@xiongmao86
Copy link
Contributor Author

xiongmao86 commented May 6, 2019

Travis CI build break because of rand crate has format_doc_comments in rustfmt.toml. What should I do next?

@scampi
Copy link
Contributor

scampi commented May 6, 2019

The code change looks good to me @xiongmao86

With the stability guaranty of rustfmt, we can't simply rename the option: rand's build failure is a good example. A possible solution would be to introduce the concept of options "aliases":

  • an option, here format_doc_comments, can be used with another name, here format_code_in_doc_comments
  • the first variant of the option's name would be marked as deprecated and removed in a major release.

@topecongiro What do you think of such a mechanism ? Just improving the format_doc_comments description could be enough for this issue, but I think a more obvious option's name would be good.

@RReverser
Copy link
Contributor

With the stability guaranty of rustfmt, we can't simply rename the option

All of these are unstable options though, so I'd expect it to be fine to play with naming / values until stabilized?

@xiongmao86
Copy link
Contributor Author

Appveyor build fatal: unable to access 'https://github.com/rust-lang/rustfmt.git/': Could not resolve host: github.com

May be failed because of network accessibility. How can I retry testing?

@scampi
Copy link
Contributor

scampi commented May 8, 2019

@RReverser ahah indeed ;oD that comment can be discarded then!

@xiongmao86
Copy link
Contributor Author

xiongmao86 commented May 8, 2019

@RReverser @scampi, then how to deal with rand's option if I switch to format_code_in_doc_comment?

And this pull request will separate wrap_comments with format_code_in_doc_comment that would cause code that imply format_code_in_doc_comment when wrap_comments is set to break, How to deal with that problem?

@scampi
Copy link
Contributor

scampi commented May 8, 2019

I believe mentioning the name change in the CHANGELOG should be good enough.

How to deal with that problem?

Given both options are unstable, nothing needs to be done. The behaviour of wrap_comments implying format_code_in_doc_comment was wrong to begin with.

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.

LGTM, sorry for the late review.

@xiongmao86
Copy link
Contributor Author

@RReverser @scampi @topecongiro, Thank you for helping and reviewing my commit. What should I do to get the pull request merge?

@RReverser
Copy link
Contributor

RReverser commented May 9, 2019

Given the discussion above, I'd say rename the option back to format_code_in_doc_comments? (I see you reverted that change)

@xiongmao86
Copy link
Contributor Author

Only rand build in Travis CI failed. I think this is probably good to go. @scampi, @topecongiro, What do you think?

@topecongiro topecongiro added this to the 1.3.0 milestone May 16, 2019
@topecongiro topecongiro merged commit 531b2d9 into rust-lang:master May 16, 2019
@topecongiro
Copy link
Contributor

@xiongmao86 Sorry for the late review. LGTM, thanks!

@xiongmao86 xiongmao86 deleted the issue3417 branch May 16, 2019 13:02
christophermaier added a commit to habitat-sh/habitat that referenced this pull request Jul 31, 2019
christophermaier added a commit to habitat-sh/habitat that referenced this pull request Jul 31, 2019
christophermaier added a commit to habitat-sh/builder that referenced this pull request Jul 31, 2019
christophermaier added a commit to habitat-sh/builder that referenced this pull request Jul 31, 2019
christophermaier added a commit to habitat-sh/habitat that referenced this pull request Aug 5, 2019
christophermaier added a commit to habitat-sh/habitat that referenced this pull request Aug 5, 2019
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.

wrap_comments shouldn't imply format_doc_comments
4 participants