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 option to preserve newline gaps for blocks #857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

InoUno
Copy link

@InoUno InoUno commented Mar 30, 2024

Adds an option that allows preservation of newline gaps at the start and/or end of blocks - see example at the end of PR.

This is similar to how newline gaps are already preserved between statements. Namely that the existence of newline gaps between statements is also dependent on if there's a gap in the input string:

local x = 1 -- the below gap is preserved

local y = 2

I'm aware that there's discussion about if more config should be added in #620, so this PR can serve as another option suggestion.

Adding this option solves this issue, but in a more general way for all blocks, instead of just for if-else-if chains.

This could instead be implemented on a subset of newline gaps (i.e. only for leading newline gaps before else-if/else tokens), or even enforced always with a different option, as suggested in the above issue.
However, I went with the more general solution in this PR, since notably gofmt also preserves newline gaps at the start and end of blocks in this way as well. It is also a quite opiniated formatter, but doesn't have an opinion on those gaps (similar to statement gaps), besides them being at most a 1-newline gap.

Motivation and example

In our codebase, we occasionally have some long if-else-if chains, and they become quite hard to read when there's no newline gaps allowed in them. This is especially true when the condition itself is multiline, since it then becomes indented at the same level as the inner block statements:

if
    this_is == very_long_variable_name
    and to_ensure_that == it_is_broken_into
    and multiple_lines == in_order_to_see_how_that_looks
then
    do_this_thing = 0
elseif
    this_is == very_long_variable_name
    and to_ensure_that == it_is_broken_into
    and multiple_lines == in_order_to_see_how_that_looks
then
    do_something_here = 1
    with_a_few_long_lines_as_well = 2
elseif
    this_is == very_long_variable_name
    and to_ensure_that == it_is_broken_into
    and multiple_lines == in_order_to_see_how_that_looks
then
    do_something_else_here = 3
    with_a_few_long_lines_as_well = 4
else
    return 1
end

Having newline gaps makes it a bit easier to distinguish where the different if-blocks start and end:

if
    this_is == very_long_variable_name
    and to_ensure_that == it_is_broken_into
    and multiple_lines == in_order_to_see_how_that_looks
then
    do_this_thing = 0

elseif
    this_is == very_long_variable_name
    and to_ensure_that == it_is_broken_into
    and multiple_lines == in_order_to_see_how_that_looks
then
    do_something_here = 1
    with_a_few_long_lines_as_well = 2

elseif
    this_is == very_long_variable_name
    and to_ensure_that == it_is_broken_into
    and multiple_lines == in_order_to_see_how_that_looks
then
    do_something_else_here = 3
    with_a_few_long_lines_as_well = 4

else
    return 1
end

@InoUno
Copy link
Author

InoUno commented Nov 27, 2024

@JohnnyMorganz Is adding this option something that's on the table now that #620 is decided? If so, I'll look into fixing the recently emerged conflicts.

@JohnnyMorganz
Copy link
Owner

Sorry, I never got round to looking at this PR. This one is interesting, I didn't realise gofmt also preserved it like this, thanks for that link.

I'm not a big fan on options to 'preserve' syntax (although we do have options to do this already, like for function call parentheses, since that was a Hot Topic). Mainly because it makes it more difficult to apply consistent reproducible formatting without human intervention. This reopens the door of stylistic nit comments in e.g. code review - now if you accidently leave a newline at the end, you have to manually go in and remove it. Ideally the formatter should take a block and format it consistently (with a set of configurable options to tweak how it can look that are decided ahead of time and stored in a config file, not decided per-code-review).

But, to be fair, gating it behind an option does alleviate most of that concern - because you'll have to make an explicit decision to opt-in to this behaviour.

Before committing to this most general solution where lines are determined ad-hoc, what do you think about the alternative option to always add a newline at the start / end of a block? Or would that be overly verbose in some cases?

@InoUno
Copy link
Author

InoUno commented Dec 7, 2024

I'm not a big fan on options to 'preserve' syntax (although we do have options to do this already, like for function call parentheses, since that was a Hot Topic). [...] Ideally the formatter should take a block and format it consistently [...]

I generally agree that this would be ideal, but I don't think it's practical in all cases, because readability of code is sometimes determined by the content of the code.

I see these newline gaps for start/end of blocks to be very similar to newline gaps between statements, which most formatters preserve at least 1 of (StyLua included currently). The existence of these gaps depends on the semantics/meaning of the code for how they fit in to split up chunks of code, and it can't be determined from just the AST (without some crazy semantic analysis). Small example:

-- Configure a
a.x = 1
a.y = 2

-- Configure b
b.x = 3
b.y = 4

Before committing to this most general solution where lines are determined ad-hoc, what do you think about the alternative option to always add a newline at the start / end of a block? Or would that be overly verbose in some cases?

I personally think that would be overly verbose, if it's done for both start and end for all blocks.
But instead of doing this as an alternative, what about just adding these 3 extra cases as options?

So the options would be like:

pub enum BlockNewlineGaps {
    /// Never allow leading or trailing newline gaps for blocks
    #[default]
    Never,
    /// Preserve both leading and trailing newline gaps for blocks, if present in input
    PreserveBoth,
    /// Preserve leading newline gaps for blocks, if present in input
    PreserveLeading,
    /// Preserve trailing newline gaps for blocks, if present in input
    PreserveTrailing,
    /// Always add both leading and trailing newline gaps for blocks
    AlwaysBoth,
    /// Always add a leading newline gaps for blocks
    AlwaysLeading,
    /// Always add trailing newline gaps for blocks
    AlwaysTrailing,
}

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.

2 participants