godotengine / godot-vscode-plugin

Godot development tools for VSCode
MIT License
1.55k stars 163 forks source link

Formatter only keeps 1 blank line between functions, but the GDScript style guide mandates 2 #612

Open Calinou opened 7 months ago

Calinou commented 7 months ago

Godot version

4.2.1.stable

VS Code version

1.86.2

Godot Tools VS Code extension version

2.0.0

System information

Fedora 39

Issue description

The GDScript formatter only keeps 1 blank line between functions, but the GDScript style guide mandates 2.

func _on_sdfgi_option_button_item_selected(index: int) -> void:
    # This is a setting that is attached to the environment.
    # If your game requires you to change the environment,
    # then be sure to run this function again to make the setting effective.
    if index == 0: # Disabled (default)
        world_environment.environment.sdfgi_enabled = false
    if index == 1: # Low
        world_environment.environment.sdfgi_enabled = true
        RenderingServer.gi_set_use_half_resolution(true)
    if index == 2: # High
        world_environment.environment.sdfgi_enabled = true
        RenderingServer.gi_set_use_half_resolution(false)

func _on_glow_option_button_item_selected(index: int) -> void:
    # This is a setting that is attached to the environment.
    # If your game requires you to change the environment,
    # then be sure to run this function again to make the setting effective.
    if index == 0: # Disabled (default)
        world_environment.environment.glow_enabled = false
    if index == 1: # Low
        world_environment.environment.glow_enabled = true
    if index == 2: # High
        world_environment.environment.glow_enabled = true

Is turned into:

func _on_sdfgi_option_button_item_selected(index: int) -> void:
    # This is a setting that is attached to the environment.
    # If your game requires you to change the environment,
    # then be sure to run this function again to make the setting effective.
    if index == 0: # Disabled (default)
        world_environment.environment.sdfgi_enabled = false
    if index == 1: # Low
        world_environment.environment.sdfgi_enabled = true
        RenderingServer.gi_set_use_half_resolution(true)
    if index == 2: # High
        world_environment.environment.sdfgi_enabled = true
        RenderingServer.gi_set_use_half_resolution(false)

func _on_glow_option_button_item_selected(index: int) -> void:
    # This is a setting that is attached to the environment.
    # If your game requires you to change the environment,
    # then be sure to run this function again to make the setting effective.
    if index == 0: # Disabled (default)
        world_environment.environment.glow_enabled = false
    if index == 1: # Low
        world_environment.environment.glow_enabled = true
    if index == 2: # High
        world_environment.environment.glow_enabled = true

Note that this only applies between functions/classes, not between top-level member variables and functions/classes.

Steps to reproduce

Format the above code using Ctrl + Shift + I.

DaelonSuzuka commented 6 months ago

Technically this isn't a bug: I think two newlines between functions is bad and the official style guide is wrong.

But... I recognize that I'm the weirdo here, so I'll change the default behavior to do 2 newlines.

limbonaut commented 6 months ago

Wouldn't it be nice to have a configuration option?

DaelonSuzuka commented 6 months ago

I've been trying to avoid adding options for the formatter because that dramatically increases the complexity of both the formatter and the test suite.

This probably will be an option, because I really don't want two newlines in my projects.

DaelonSuzuka commented 6 months ago

Code_BQ519Gn3mh

Progress. It doesn't handle comments/doc comments yet, and it doesn't insert blanks lines if they're missing. And it breaks half the tests.

chrisl8 commented 6 months ago

Technically this isn't a bug: I think two newlines between functions is bad and the official style guide is wrong.

But... I recognize that I'm the weirdo here, so I'll change the default behavior to do 2 newlines.

FWIW, I think Godot is attempting to follow the Python style guide.

https://peps.python.org/pep-0008/#blank-lines

Surround top-level function and class definitions with two blank lines.

Calinou commented 6 months ago

FWIW, I think Godot is attempting to follow the Python style guide.

Indeed, that was the main rationale for using 2 blank lines between functions/classes. There's also the reason that it mimics the effective spacing between functions in languages with brace syntax:

void something() {
    // pass
}

void other_function() {
    // pass
}
def something():
    pass

def other_function():
    pass

In both examples above, there are 2 "blank" lines between functions (even if in the brace-based case, there's a lone brace on the first line).

rktprof commented 6 months ago

Would it be possible to make it add lines but never remove lines? Sometimes I like to separate blocks with more than 1 or 2 lines

Calinou commented 6 months ago

Would it be possible to make it add lines but never remove lines? Sometimes I like to separate blocks with more than 1 or 2 lines

It's possible to design a formatter this way, but I don't think it's a good idea as it goes against the style guide (on top of making the formatter code more complex and bug-prone).

rktprof commented 6 months ago

It's possible to design a formatter this way, but I don't think it's a good idea as it goes against the style guide (on top of making the formatter code more complex and bug-prone).

Understandable, in this specific case it just feels more like a style enforcement rather than a guide =)

limbonaut commented 5 months ago

In defense of two blank lines... I tend to use blank lines to break up longer functions into logical subsections. With a single blank line between functions, it becomes harder to see at a glance where one function ends and another starts.