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

Update code order with static annotation on gdscript_styleguide.rst #8920

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

Conversation

Xinart
Copy link

@Xinart Xinart commented Feb 7, 2024

Update code order on gdscript_styleguide.rst
by inserting static variables and methods on the code order as a proposal

Update code order on gdscript_styleguide.rst
by inserting static variables and methods on the code order as a proposal
@AThousandShips AThousandShips requested a review from a team February 7, 2024 16:24
@AThousandShips AThousandShips added enhancement topic:gdscript area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Feb 7, 2024
@dalexeev
Copy link
Member

dalexeev commented Feb 7, 2024

Personally, I prefer to place static members before non-static ones, so I would suggest the following order:

01. @tool, @icon, @static_unload
02. class_name
03. extends
04. ## class doc comment

05. signals
06. enums
07. constants
08. static public variables
09. static private variables
10. @export variables
11. public variables
12. private variables
13. @onready variables

14. static public methods
15. static private methods
16. optional built-in virtual _init method
17. optional built-in virtual _enter_tree() method
18. built-in virtual _ready method
19. remaining built-in virtual methods
20. public methods
21. private methods
22. subclasses

(Actually, I use a slightly different order.)

@Xinart
Copy link
Author

Xinart commented Feb 8, 2024

Ok currently i have been thinking of changing this part a bit differently to be better understanble and to not overweight the code order with the access modofiers beceause it makes doublons and harder to read, i'll do a litle update

@Xinart
Copy link
Author

Xinart commented Feb 8, 2024

Personally, I prefer to place static members before non-static ones, so I would suggest the following order:

01. @tool, @icon, @static_unload
02. class_name
03. extends
04. ## class doc comment

05. signals
06. enums
07. constants
08. static public variables
09. static private variables
10. @export variables
11. public variables
12. private variables
13. @onready variables

14. static public methods
15. static private methods
16. optional built-in virtual _init method
17. optional built-in virtual _enter_tree() method
18. built-in virtual _ready method
19. remaining built-in virtual methods
20. public methods
21. private methods
22. subclasses

(Actually, I use a slightly different order.)

I Agree with what you said beceause they should be easily findable

I implemented @dalexeev proposal and i simplified the code order by splitong the access modifiers order specification, the overrided methods from Godot base class and renaming some explanation to make it clearer
I just rearanged the code order i missed to do in last commit
@Xinart
Copy link
Author

Xinart commented Feb 12, 2024

@AThousandShips I commited your reviews, is it fine now to merge the branch ?

@AThousandShips
Copy link
Member

It needs an approval first

@Xinart Xinart closed this Mar 1, 2024
@Xinart Xinart reopened this Mar 1, 2024
@Xinart
Copy link
Author

Xinart commented Mar 1, 2024

Is it possible to have an approval on this to merge the branch @AThousandShips ?

@AThousandShips
Copy link
Member

That's dalexeev's area :)

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

There seems to be no objection. Just a couple of minor comments. Thank you!

@@ -715,26 +715,38 @@ We suggest to organize GDScript code this way:

::

01. @tool
01. ``@tool``, ``@icon``, ``@static_unload``
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
01. ``@tool``, ``@icon``, ``@static_unload``
01. @tool, @icon, @static_unload

Sorry, I didn't notice that the list is already inside the code block.

16. public methods
17. private methods
18. subclasses
12. ``_static_init()``
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
12. ``_static_init()``
12. _static_init()

Comment on lines +728 to 729
10. remaining variables
11. @onready variables
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
10. remaining variables
11. @onready variables
10. remaining regular variables
11. @onready variables

I'm not sure how best to say "except @onready".

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 enhancement topic:gdscript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants