Skip to content

Format comments with different opening in different manner #1604

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 2 commits into from
May 29, 2017

Conversation

topecongiro
Copy link
Contributor

This PR closes #691.
However, TBH I am not sure whether how I deal with this issue is ideal.
Introducing enum CommentStyle seems overkill after looking back this PR.
I really appreciate any feedbacks.
Thank you in advance!

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 for the PR, I think the overall design is good and factoring out the CommentStyle enum is a good call. I have a couple of places where this might be improved in small ways.

src/comment.rs Outdated
(self.opener(orig), self.closer(), self.line_start(orig))
}

pub fn to_pattern<'a>(&self, line: &str, orig: &'a str, normalize_comments: bool) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this function is doing, I think the name is not ideal because I would expect to_pattern to convert a CommentStyle into a Pattern.

src/comment.rs Outdated
if rest.is_empty() {
Some(first_group_str)
} else {
rewrite_comment(&rest, block_style, shape, config).map(|rest_str| {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than have rewrite_comment and rewrite_comment_inner where the latter recurses, could we have a rewrite_comment function which calls an identify_comment where identify_comment is recursive?

@topecongiro
Copy link
Contributor Author

Thank you! Rebased reflecting the feedbacks.

  1. renamed to_pattern to line_with_same_comment_style.
  2. created identify_comment and made it recursive.

-> Option<String> {
let style = comment_style(orig, false);
let first_group = orig.lines()
.take_while(|l| style.line_with_same_comment_style(l, orig, false))
Copy link
Contributor Author

@topecongiro topecongiro May 29, 2017

Choose a reason for hiding this comment

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

I was wondering if rust has a function equivalent to span in haskell.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is one - there might be one in Itertools, but I prefer not to link it and use simpler functions.

Copy link
Member

Choose a reason for hiding this comment

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

You might be able to use partition

@nrc nrc merged commit b790942 into rust-lang:master May 29, 2017
@nrc
Copy link
Member

nrc commented May 29, 2017

Thanks for the changes!

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.

Incorrect comment reformat
2 participants