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

Mixed tabs and spaces no longer supported for multiline declarations #30937

Closed
nobuyukinyuu opened this issue Jul 29, 2019 · 15 comments · Fixed by #32808
Closed

Mixed tabs and spaces no longer supported for multiline declarations #30937

nobuyukinyuu opened this issue Jul 29, 2019 · 15 comments · Fixed by #32808
Assignees
Milestone

Comments

@nobuyukinyuu
Copy link
Contributor

nobuyukinyuu commented Jul 29, 2019

Godot version:
6c9ccf1

Issue description:
GDScript files which contain mixed tabs and spaces in multiline definitions (for example, to align dictionaries or arrays) are now considered a hard error when before they were ignored. This can (for example) break plugins when migrating.

@akien-mga
Copy link
Member

Can you give examples of such multiline definitions?

@nobuyukinyuu
Copy link
Contributor Author

can-do.

image

I had to replace 3 spaces on lines 21 and 22 to a tab for the script to pass the current parser, but it now looks ugly. This is just a simple example; more complicated ones would obviously have more instances of this necessary to alignment if nesting becomes involved (such as with dictionaries) and the declaration far exceeded 80 characters (or whatever the canonical convention is supposed to be now).

Another few examples flagged in my current codebase:
image
image
image

@TheDuriel
Copy link
Contributor

Mixing tabs and spaces should error. The convention commonly seen for continuing lines is two tabs anyways.

@akien-mga
Copy link
Member

akien-mga commented Jul 29, 2019

It should only error when the indentation has syntactic meaning, like in a code block. So that last screenshot is a valid error for example, the comment is badly indented in its scope.

But for array/dict declarations and statements wrapped over multiple lines, freestyle should be allowed (even if mixing tabs and spaces is ewww, but still not a reason for a syntax error if it can't be a source of bugs).

@Xrayez
Copy link
Contributor

Xrayez commented Jul 30, 2019

This can (for example) break plugins when migrating.

True, I had to come up with custom indentation fixes in order to continue using some plugins, see bitwes/Gut#126 for instance. Also noticed this in godot-logger.

@Xrayez
Copy link
Contributor

Xrayez commented Jul 30, 2019

Talking about plugins, consider making an ability to ignore indentation inconsistencies for "third-party" stuff or custom folders, as proposed in #28232 to ignore warnings in GDScript. Alternatively, one could make indentation inconsistency error to be treated as a warning optionally (per project or folder). Warnings can be turned on to be treated as errors anytime, though you can't make a single warning to be treated as an error unfortunately.

@Skaruts
Copy link

Skaruts commented Aug 2, 2019

@akien-mga Also for long function calls that one may want to break the parentheses into several lines.

@Calinou
Copy link
Member

Calinou commented Aug 2, 2019

@Skaruts This can already be done using only tabs (or spaces), for instance:

some_function_call(
        123, "hello", 456)

some_function_call(
        123,
        "hello",
        456
)

The GDScript style guide advises using two tabs to distinguish line continuation indents from regular code blocks.

@Skaruts
Copy link

Skaruts commented Aug 2, 2019

@Calinou Yea, sometimes I do them like that (the 2nd one), but usually with the ) indented one more (because the two tabs on their own don't make the relationship between the lines immediately stand out to me).

But there are cases where doing them like the example below helps me visualize things better. The code looks clearer to my eyes this way.

igm.draw_linestrip( 
                    [   
                        Vector3(), 
                        Vector3(core.grid_size, 0,  0),
                        Vector3(core.grid_size, 0, core.grid_size), 
                        Vector3(0 , 0, core.grid_size),
                        Vector3()
                    ], 
                    "ig_grid_bounds"     # <-- second function parameter
                  ) 

(Had to use spaces there just to align things properly, because github. The original uses tabs.)
I usually need spaces to align those.

Also, I realize that could be simplified by building the array separately, but I don't remember where I would have a better example right now.

@michelmontenegro
Copy link

Who program in other languages knows that "Tab" is used and always was to identify code, now in cases of use of matrix or similar even to think, but still, what is the justification?

What is the justification? Just say it's like that and that's it, no real advantage?

@michelmontenegro
Copy link

Why don't you come back and release the TAB?
Removing Godot's TAB usage is very wrong, all programming languages coexist well, why did Godot decide to change that?

@akien-mga
Copy link
Member

akien-mga commented Oct 12, 2019

What is the justification for [issued described and labelled as a bug]?

It's a bug.

@nobuyukinyuu
Copy link
Contributor Author

This bug has returned as of v4.0.dev.custom_build.364ea7f28. example:
image

Please consider re-opening the issue if the cause is similar.

@Calinou
Copy link
Member

Calinou commented May 28, 2021

This bug has returned as of v4.0.dev.custom_build.364ea7f28. example:
image

Please consider re-opening the issue if the cause is similar.

The GDScript implementation is brand new in 4.0, so I'd recommend opening a new issue instead.

@fcole90
Copy link

fcole90 commented May 30, 2021

See my workaround at #49171 (comment) and my proposal to revise the indentation guidelines: godotengine/godot-proposals#2800

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants