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 wrapping example using parentheses and backslashes #9156

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gmikhail
Copy link
Contributor

@AThousandShips AThousandShips added enhancement topic:gdscript area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Mar 29, 2024
@AThousandShips AThousandShips requested a review from a team March 29, 2024 14:51
gmikhail and others added 2 commits March 29, 2024 16:53
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@AThousandShips
Copy link
Member

I'm unsure if we should suggest the parentheses version, we discourage unnecessary parentheses and these are unnecessary as the continuation method exists

Let's get an opinion from the GDScript team

@gmikhail
Copy link
Contributor Author

gmikhail commented Apr 2, 2024

Let me clarify that the main goal of this PR is to demonstrate all possible line wrapping options.

You can highlight a recommended option, but it's good practice to show all the available options.

@AThousandShips
Copy link
Member

Sure, but the same page says using parentheses unnecessarily is bad practice, so it doesn't make sense to me to have a later example that contradict it, parentheses around the conditions is not GDScript style, this is a style guide, not instructions on how to wrap lines in general IMO

@mhilbrunner
Copy link
Member

cc @dalexeev / @godotengine/gdscript

@dalexeev
Copy link
Member

dalexeev commented Apr 2, 2024

I think there should only be one recommended style on this page. The fact that backslash can be used to wrap long lines is already mentioned in the GDScript reference: one, two.

But indeed, the style guide does not answer which option is preferable. Looking at GDScript reference, Python, gdlint and the formatter PR, the preferred style seems to be parentheses (however, the number of tabs/spaces and the position of parentheses is unclear). This option is also more VCS-friendly.

On the other hand, when adding a new rule to the style guide, we should look at the established practices in the community. What style do we use in demo projects, what style do popular addons and projects use?

Comment on lines +362 to +365
if ($Sprite.animation == "back_crouch"
or $Sprite.animation == "front_crouch"
or $Sprite.animation == "horizontal_crouch"):
do_something()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    if ($Sprite.animation == "back_crouch"
            or $Sprite.animation == "front_crouch"
            or $Sprite.animation == "horizontal_crouch"
    ):
        do_something()

or

    if (
            $Sprite.animation == "back_crouch"
            or $Sprite.animation == "front_crouch"
            or $Sprite.animation == "horizontal_crouch"
    ):
        do_something()

or

    if (
        $Sprite.animation == "back_crouch"
        or $Sprite.animation == "front_crouch"
        or $Sprite.animation == "horizontal_crouch"
    ):
        do_something()

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to use the second one personally, as it's what the GDScript style guide recommends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.1 cherrypick:4.2 enhancement topic:gdscript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants