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

Fix bad formatting on several operators #605

Merged
merged 37 commits into from
Jun 24, 2024

Conversation

DaelonSuzuka
Copy link
Collaborator

@DaelonSuzuka DaelonSuzuka commented Feb 25, 2024

Fixes #601, #602, and #609

Before this is merged, it's probably worth going through all of GDScripts operators and making sure they're represented in the snapshot tests.

@Calinou
Copy link
Member

Calinou commented Feb 29, 2024

Note that in master, this is also currently being incorrectly formatted:

func update_preset() -> void:
	# Simulate options being manually selected to run their respective update code.
	%TAAOptionButton.item_selected.emit(%TAAOptionButton.selected)
	%MSAAOptionButton.item_selected.emit(%MSAAOptionButton.selected)
	%FXAAOptionButton.item_selected.emit(%FXAAOptionButton.selected)
	%ShadowSizeOptionButton.item_selected.emit(%ShadowSizeOptionButton.selected)
	%ShadowFilterOptionButton.item_selected.emit(%ShadowFilterOptionButton.selected)
	%MeshLODOptionButton.item_selected.emit(%MeshLODOptionButton.selected)
	%SDFGIOptionButton.item_selected.emit(%SDFGIOptionButton.selected)
	%GlowOptionButton.item_selected.emit(%GlowOptionButton.selected)
	%SSAOOptionButton.item_selected.emit(%SSAOOptionButton.selected)
	%SSReflectionsOptionButton.item_selected.emit(%SSReflectionsOptionButton.selected)
	%SSILOptionButton.item_selected.emit(%SSILOptionButton.selected)
	%VolumetricFogOptionButton.item_selected.emit(%VolumetricFogOptionButton.selected)

The formatter will turn this into:

func update_preset() -> void:
	# Simulate options being manually selected to run their respective update code.
	%TAAOptionButton.item_selected.emit( %TAAOptionButton.selected)
	%MSAAOptionButton.item_selected.emit( %MSAAOptionButton.selected)
	%FXAAOptionButton.item_selected.emit( %FXAAOptionButton.selected)
	%ShadowSizeOptionButton.item_selected.emit( %ShadowSizeOptionButton.selected)
	%ShadowFilterOptionButton.item_selected.emit( %ShadowFilterOptionButton.selected)
	%MeshLODOptionButton.item_selected.emit( %MeshLODOptionButton.selected)
	%SDFGIOptionButton.item_selected.emit( %SDFGIOptionButton.selected)
	%GlowOptionButton.item_selected.emit( %GlowOptionButton.selected)
	%SSAOOptionButton.item_selected.emit( %SSAOOptionButton.selected)
	%SSReflectionsOptionButton.item_selected.emit( %SSReflectionsOptionButton.selected)
	%SSILOptionButton.item_selected.emit( %SSILOptionButton.selected)
	%VolumetricFogOptionButton.item_selected.emit( %VolumetricFogOptionButton.selected)

I'm not sure if fixing this is within the scope of this PR, but I figured I'd mention it nonetheless.

@DaelonSuzuka
Copy link
Collaborator Author

That's awesome, thanks @Calinou!

@Mr-Byte
Copy link

Mr-Byte commented Mar 3, 2024

I ran into an issue that seems related where the following code:

func test(x):
    match x:
         var y when y > 20:
            pass

Gets formatted into:

func test(x):
    match x:
         var ywheny > 20:
            pass

Which unfortunately causes the formatted code to become invalid.

@DaelonSuzuka
Copy link
Collaborator Author

I don't think I've seen a "when" keyword in GDScript. Thanks for reporting!

@DaelonSuzuka
Copy link
Collaborator Author

@Calinou it looks like when isn't in the list of keywords here: https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_basics.html#keywords

Is this table supposed to be exhaustive?

@DaelonSuzuka
Copy link
Collaborator Author

The when keyword is now syntax highlighted and formatted correctly.

image

@DaelonSuzuka
Copy link
Collaborator Author

%Unique node shorthand inside parens now formats correctly. (Actually, everything following an open paren should format correctly now.)

image

@DaelonSuzuka
Copy link
Collaborator Author

Working on fix for #612:

Code_BQ519Gn3mh

This breaks half the tests, it doesn't properly handle comments, and it doesn't insert blank lines if they're missing.

@jogly
Copy link
Contributor

jogly commented Mar 13, 2024

I also confirmed this fixes the currently broken formatting of the bitwise operators >> and <<! ty <3

@rktprof
Copy link

rktprof commented Mar 18, 2024

Heads up, this breaks some variable names, namely variables starting with "and" or "or" gets formatted

for example:

func _ready() -> void:
	var origin: Vector2 = Vector2.ZERO
	origin.x = 1

becomes:

func _ready() -> void:
	var origin: Vector2 = Vector2.ZERO
	or igin.x = 1

@DaelonSuzuka
Copy link
Collaborator Author

@rktprof Thanks for the report.

@DaelonSuzuka
Copy link
Collaborator Author

@rktprof

Fixed the issue with and/or/not in identifiers:

image

@DaelonSuzuka
Copy link
Collaborator Author

Fixed consecutive trailing line removal:

image

@DaelonSuzuka
Copy link
Collaborator Author

Fixed bad bitwise operator formatting:

image

@DaelonSuzuka
Copy link
Collaborator Author

@Calinou

@limbonaut is right, this PR isn't ready. There's a number of outstanding problems but I should finally have some free time to work on it this week.

@limbonaut

Wanted to report another reformatting accident:
func _add_action(p_action: StringName, p_key: Key, p_alt: Key = KEY_NONE) -> void:
=>
func _add_action(p_action: StringName, p_key: Key, p_alt: Key=KEY_NONE) -> void:

Doesn't look right to me.

This is actually not a bug, I prefer removing the whitespace inside parameter lists for aesthetic/space reasons, and because that's the idiomatic style for several other languages I use.

However, enough people have thought this was a bug that I'm considering changing it.

@limbonaut
Copy link

@DaelonSuzuka
It only looks strange with static typing. When no types provided, that looks fine, like in some other languages that support default argument values. With types, it kinda looks random, though.

I checked with black - python's formatter - to see how it handles such cases. It removes whitespace when no types involved, and leaves single space when types are provided.

def func(arg=5):
    pass

def func(arg: int = 5):
    pass

Hope it helps.

@DaelonSuzuka
Copy link
Collaborator Author

I have previously stated that I'm against configuration options, but I think that's the least wrong thing to do here. There's now two densities for function (and lambda) parameter lists, dense and not dense. The default is dense==false, which matches the Godot style guide. The dense mode should match what black does (thanks to @limbonaut for correcting my faulty memory about my main programming language T_T).

@phelioz
Copy link

phelioz commented Jun 24, 2024

Formater breaks bitwise OR assignment operator.
Turns:

test |= 1

Into:

test | = 1

@limbonaut
Copy link

limbonaut commented Jun 24, 2024

With the latest build and two empty lines setting, I've got the following issue related to comments:

extends Node
## Class doc


func _ready() -> void:
    pass


func no_comment() -> void:
    pass


## Doc comment
func with_doc_comment() -> void:
    pass


# Comment
func with_comment() -> void:
    pass


## Multiline doc comment
## Second line
func with_multiline_doc_comment() -> void:
    pass


static func a_static_func() -> void:
  pass

Reformatted to:

extends Node
## Class doc


func _ready() -> void:
    pass


func no_comment() -> void:
    pass

## Doc comment
func with_doc_comment() -> void:
    pass

# Comment
func with_comment() -> void:
    pass

## Multiline doc comment
## Second line
func with_multiline_doc_comment() -> void:
    pass

static func a_static_func() -> void:
  pass

So it seems that two lines setting work, unless a function declaration has comments preceding.

UPDATE: and also static functions don't respect two-lines setting.

@limbonaut
Copy link

Negative indexing issue - space introduced

_array[-1] = 0_array[- 1] = 0

Typed array arguments don't follow density rules

func _init(m: Array[int] = []) -> void:
↓↓↓
func _init(m: Array[int]=[]) -> void:

Inline documentation comments get broken

enum MyEnum { ## My enum.
	VALUE_A = 0, ## Value A.
	VALUE_B = 1, ## Value B.
}

↓↓↓

enum MyEnum { # # My enum.
	VALUE_A = 0, # # Value A.
	VALUE_B = 1, # # Value B.
}

Negative numbers in function arguments

func negnum(number = -1) -> void:
↓↓↓
func negnum(number =- 1) -> void:

Note: Same issue if types are specified.

@limbonaut
Copy link

Here's all these issues in a single GDScript file: test_formatter.zip

@DaelonSuzuka
Copy link
Collaborator Author

DaelonSuzuka commented Jun 24, 2024

Alright, @limbonaut's test cases are (almost) all implemented. It's time to merge this and start arranging a new release.

I'm sure there will be a new "general formatter work" PR soon, so additional feedback can either open new issues or wait for the new thread.

@DaelonSuzuka DaelonSuzuka merged commit c07fe37 into godotengine:master Jun 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GDScript formatter removing spaces around "||"
9 participants