-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(fmt): don't normalize single-line non-doc block cmnts #12078
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
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ | ||
/* FUNCTIONS */ | ||
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ |
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.
when wrapping comments we need to split the comment content into words, and separate them with soft breaking spaces.
if a user has comment wrapping enabled and they want to do custom art with their comments, they may have to disable the fmt for that block.
imo this is totally fine, as custom style conflicts with comment wrapping
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.
does this mean that you can't have more than 1 consecutive space inside of a comment when wrapping is enabled? we can ignore this here but that doesn't sound right
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.
since when wrapping comments we defer the breaking logic to the PP, and we wouldn't know when it would break, the current approach is to split cmnt content into words, stripping whitespaces and exchanging them for a single PP breaking space:
foundry/crates/fmt/src/state/mod.rs
Line 620 in 8e3d10e
let mut words = content.split_whitespace().peekable(); |
i can change the approach and go char by char though if we are not happy with this approach
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.
addressed in 6983489
(#12078)
crates/fmt/src/state/mod.rs
Outdated
} | ||
let prefix = post_break_prefix(prefix, rest.len()); | ||
(prefix, rest) | ||
} else if line.starts_with("/*") && !line.starts_with("/* ") { |
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.
cannot this happen on comments starting with //
or ///
?
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.
afaik doc comments must have the space afterwards, otherwise they are not identified as doc comments.
personally i felt like //*
was weird, but i can modify the patch to allow it too. Should i do it @grandizzy? addressed in 6983489
(#12078)
bd8c8d9
to
6983489
Compare
crates/fmt/src/state/mod.rs
Outdated
let content = &line[prefix.len()..]; | ||
let content = if is_doc { | ||
// Doc comments preserve leading whitespaces (right after the prefix) as nbps. | ||
let ws_len = content.chars().take_while(|&c| c.is_whitespace()).count(); |
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 is char count instead of byte count
…#12078) * fix: don't normalize single-line non-doc block cmnts * fix: preserve consecutive whitespaces * fix: use byte length instead of char count
* fix(fmt): ensure commasep breaks with final trailing cmnt (#12031) * fix(fmt): refine logic over comments between uninformed commasep (#12055) * fix(anvil): set envelope for non deposit tx in debug tracers (#12080) * chore: bump v1.4.1 (#12083) chore: bumpt v1.4.1 * fix(fmt): don't normalize single-line non-doc block cmnts (#12078) * fix: don't normalize single-line non-doc block cmnts * fix: preserve consecutive whitespaces * fix: use byte length instead of char count --------- Co-authored-by: 0xrusowsky <[email protected]>
Motivation
closes #12076
Solution
don't normalize single-line non-doc block comments
PR Checklist