Skip to content

Commit

Permalink
fix(formatter): fix else indentation after comment between if-else (F…
Browse files Browse the repository at this point in the history
…uelLabs#3785)

closes FuelLabs#2649

Added a few test cases with comments in between, including:
- single-line comment with overindentation,
- multi-line comment with underindentation,
- single-line comment, from inline to multiline if-else

Updated `get_post_context()` with a better way to format `else` after
comment. This is a bit of a hacky fix but since the post-context concept
is currently only used for formatting `else` blocks after comment, I was
thinking this might tide us through for now (and the test suite passes
as well).

I wrote a long-form comment explaining what the new get_post_context()
does there especially because fixing the off-by-one error with
subtracting by 1 is going to look like a hacky solution (and it is,
unfortunately)

Co-authored-by: Joshua Batty <[email protected]>
  • Loading branch information
eightfilms and JoshuaBatty authored Jan 18, 2023
1 parent acd4c1e commit 1c4f3e2
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 7 deletions.
109 changes: 109 additions & 0 deletions swayfmt/src/formatter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1375,6 +1375,115 @@ fn foo() {}
#[test]
#[inline(always)]
fn foo() {}
"#;
let mut formatter = Formatter::default();
let formatted_sway_code =
Formatter::format(&mut formatter, Arc::from(sway_code_to_format), None).unwrap();
assert_eq!(correct_sway_code, formatted_sway_code);
assert!(test_stability(formatted_sway_code, formatter));
}

#[test]
fn test_comment_between_if_else_overindented() {
let sway_code_to_format = r#"contract;
impl MyContract for Contract {
fn is_blue() -> bool {
if self == PrimaryColor::Blue {
true
}
// TODO remove this else when exhaustive ifs are checked for
else {
false
}
}
}"#;

let correct_sway_code = r#"contract;
impl MyContract for Contract {
fn is_blue() -> bool {
if self == PrimaryColor::Blue {
true
}
// TODO remove this else when exhaustive ifs are checked for
else {
false
}
}
}
"#;
let mut formatter = Formatter::default();
let formatted_sway_code =
Formatter::format(&mut formatter, Arc::from(sway_code_to_format), None).unwrap();
assert_eq!(correct_sway_code, formatted_sway_code);
assert!(test_stability(formatted_sway_code, formatter));
}

#[test]
fn test_multiline_comment_between_if_else_underindented() {
let sway_code_to_format = r#"contract;
impl MyContract for Contract {
fn is_blue() -> bool {
if self == PrimaryColor::Blue {
true
}
// TODO
// remove this else when exhaustive ifs are checked for
else {
false
}
}
}"#;

let correct_sway_code = r#"contract;
impl MyContract for Contract {
fn is_blue() -> bool {
if self == PrimaryColor::Blue {
true
}
// TODO
// remove this else when exhaustive ifs are checked for
else {
false
}
}
}
"#;
let mut formatter = Formatter::default();
let formatted_sway_code =
Formatter::format(&mut formatter, Arc::from(sway_code_to_format), None).unwrap();
assert_eq!(correct_sway_code, formatted_sway_code);
assert!(test_stability(formatted_sway_code, formatter));
}

#[test]
fn test_comment_between_if_else_inline_to_multiline() {
let sway_code_to_format = r#"contract;
impl MyContract for Contract {
fn is_blue() -> bool {
if self == PrimaryColor::Blue { true }
// TODO remove this else when exhaustive ifs are checked for
else { false }
}
}"#;

let correct_sway_code = r#"contract;
impl MyContract for Contract {
fn is_blue() -> bool {
if self == PrimaryColor::Blue {
true
}
// TODO remove this else when exhaustive ifs are checked for
else {
false
}
}
}
"#;
let mut formatter = Formatter::default();
let formatted_sway_code =
Expand Down
22 changes: 15 additions & 7 deletions swayfmt/src/utils/map/comments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,12 @@ fn get_comments_between_spans(
pre_context: unformatted_code[starting_position_for_context..comment_span.start]
.to_string(),
comment: comment.clone(),
post_context: get_post_context(unformatted_code, comment_span, next_line),
post_context: get_post_context(
unformatted_code,
starting_position_for_context,
comment_span,
next_line,
),
});
}
}
Expand Down Expand Up @@ -255,16 +260,19 @@ fn format_context(context: &str, threshold: usize) -> String {
// We need to know the context after the comment as well.
fn get_post_context(
unformatted_code: &Arc<str>,
context_start: usize,
comment_span: &ByteSpan,
next_line: &str,
) -> Option<String> {
if next_line.trim_start().starts_with("else") {
let else_token_start = next_line
.char_indices()
.find(|(_, c)| !c.is_whitespace())
.map(|(i, _)| i)
.unwrap_or_default();
Some(unformatted_code[comment_span.end..comment_span.end + else_token_start].to_string())
// We want to align the 'else' token with the above comment, so this is just
// same as the pre_context subtracted by 1.
//
// 1 here is somewhat a magic number. This is a result of the format_else_opt()
// in utils/language/expr/conditional.rs always formatting with a whitespace.
// We want to take that away since this 'else' will always be on a newline,
// and will be overindented by one whitespace if left alone.
Some(unformatted_code[context_start..(comment_span.start - 1)].to_string())
} else {
// If we don't find anything to format in the context after, we simply
// return an empty context.
Expand Down

0 comments on commit 1c4f3e2

Please sign in to comment.