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

Suggest snake_case script names by default #41991

Closed

Conversation

neqs20
Copy link

@neqs20 neqs20 commented Sep 11, 2020

Script Create Dialog will now suggest snake case file names for GDScript, VisualScript and NativeScript.

This doesn't apply to C# which requires PascalCase file names (to match the class name).

Closes: godotengine/godot-proposals#1211.

@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:editor usability labels Sep 11, 2020
@Calinou Calinou added this to the 4.0 milestone Sep 11, 2020
@capnm
Copy link
Contributor

capnm commented Sep 11, 2020

typo defualt → default

@neqs20 neqs20 changed the title Suggest snake_case script names by defualt Suggest snake_case script names by default Sep 11, 2020
@neqs20 neqs20 force-pushed the default-snake-case-for-script-names branch from 285ff45 to 78a5ccb Compare September 11, 2020 20:45
@neqs20
Copy link
Author

neqs20 commented Sep 11, 2020

typo defualt → default

Corrected. Thanks!

@capnm
Copy link
Contributor

capnm commented Sep 11, 2020

Works fine for me on the 3.2 branch

Adds _ around numbers, e.g. AnimCollision-b01 → anim_collision-b_01_.gd

After using this patch for a while, I am now personally against renaming the object script names. It causes the names to get mixed up, e.g. If you don't want or can't follow exactly the PascalCase convention.

@EricEzaM
Copy link
Contributor

I don't like how this implementation relies on a "magic string" to determine whether case should be snake or pascal. What if another language is added in the future and we want it to use this option? What if other custom languages in modules also want to make use of this option? They can't because it is defined within the core codebase. This does not follow the Open-Closed principle. Also remember that C# is not part of core - it is a module too. So if you compile Godot without the mono module, this code kind of makes no sense since C# doesn't exist.

I suggest moving this to a method in ScriptLanguage which can be overridden. This way, any language which inherits script language will be able to make use of this switch. A method like virtual bool prefers_snake_case_names() { return true; } will do. This can then be overridden where desired to return false;, such in the C# script language and any other language which is added in the future. I believe this could also be exposed to GDNative plugin script via godot_pluginscript_language_desc so that custom language bindings (i.e. rust, python, etc) could also use this option.

You would also need to apply the logic when the language is changed in ScriptCreateDialog::_lang_changed() so that when you change the language the filename is updated accordingly. You may also want to cache the original filename (or use initial_bp) so that you don't need to constantly convert between pascal and snake case as you switch languages. However, this would reset the filename when the user switches, so maybe implementing a snake case to camel/pascal case method would be better.

Here is what I did on a local branch:
master...EricEzaM:WIP/snake-case-for-script-names

I made VisualScript use the switch so I could test easily:
path_case_switch_test

@EricEzaM
Copy link
Contributor

EricEzaM commented Sep 14, 2020

@capnm underscores are added by the string method which converts camel to snake case - this would be an implementation detail of that method. Not sure if core devs would be willing to change a core method for this. I think most of the time though you would want underscores around numbers. It just so happens that in the example you gave it is not ideal with that particular naming format.

An underscore is added unnecessarily at the end because of the dot for the file extension. If the extension and the dot are removed, and then the conversion is done, it works just fine as in my gif above.

@capnm
Copy link
Contributor

capnm commented Sep 15, 2020

@capnm underscores are added by the string method which converts camel to snake case

I understand what is going on. I'm just saying that now, after practical testing in my projects, I am against this idea of automatic name change (i.e.enforcing a snake_case file naming convention). Personally, I think it's better if the filenames match the object names.

@EricEzaM
Copy link
Contributor

Fair enough - good feedback to have.

@capnm
Copy link
Contributor

capnm commented Sep 15, 2020

Looks like I'm not alone. The code of the guys around the new Godot-docs hire confirms for me, that the snake_case filename convention wasn't a smart idea. I secretly hope @NathanLovato will fix the convention documents;)

https://github.com/GDQuest/godot-2d-tactical-space-combat/tree/master/godot

@Calinou
Copy link
Member

Calinou commented Sep 15, 2020

@capnm We'd prefer keeping the snake_case recommendation as it sidesteps case sensitivity issues that can occur when exporting projects on Windows.

@capnm
Copy link
Contributor

capnm commented Sep 15, 2020

We'd prefer keeping the snake_case recommendation as it sidesteps case sensitivity issues that can occur when exporting projects on Windows.

Looks like a Godot bug to me. The editor should then not allow file names that differ only in upper and lower case and internally ignore the case in file names.

@robbertzzz
Copy link

+1 for keeping CamelCase for consistency. The alternative would be to rename all internal classes so they also follow a snake_case convention.

@Calinou
Copy link
Member

Calinou commented Sep 15, 2020

@robbertzzz Godot's C++ source files use snake_case even though the class names use PascalCase. We don't intend to change that anytime soon. Most GDScript conventions come from the C++ conventions used through Godot.

@capnm
Copy link
Contributor

capnm commented Sep 15, 2020

When you save a scene, the dialog also uses the PascalCase name from the root node ...
I think it would be better to fix the case issues than to force users to use lowercase filenames
for scripts. The C++ convention is a different domain.

@NathanLovato
Copy link
Contributor

NathanLovato commented Sep 15, 2020

The convention has been PascalCase node, scene, and gdscript file names since the first release of Godot in the editor.

I'm not against snake_case for all files, but if that's the way you're expected to name all files, then the editor should take care of that for all files. Otherwise, you're asking the user to work against the program.

@NathanLovato
Copy link
Contributor

Ah, and I second that this repo is the engine's back-end, a separate domain, so its conventions don't have to be applied to and affect the user's experience when working with the editor. I mean that consistency with the C++ back-end doesn't have to be part of the equation; it's a separate project.

@Shatur
Copy link
Contributor

Shatur commented Mar 23, 2021

When you save a scene, the dialog also uses the PascalCase name from the root node ...

Should be fixed in #41991.

@fire
Copy link
Member

fire commented Feb 6, 2022

@Calinou I think this one is fixed.

@Calinou
Copy link
Member

Calinou commented Feb 6, 2022

@Calinou I think this one is fixed.

I've tested the current behavior on master 9d1626b. While this is fixed when the scene file is already saved, this is not fixed yet when the scene isn't saved to disk yet and you attach a script to the root node:

image

This is also not fixed yet when adding a script to a node that isn't the scene root. Therefore, this PR is still relevant 🙂

@akien-mga
Copy link
Member

This should be re-assessed against the current 4.0 branch, I think this is now the default behavior for scenes already, but maybe not yet for scripts.

CC @Calinou @KoBeWi

@akien-mga akien-mga removed this from the 4.0 milestone Jul 28, 2022
@akien-mga akien-mga added this to the 4.x milestone Jul 28, 2022
@KoBeWi
Copy link
Member

KoBeWi commented Aug 14, 2022

Ok so

@akien-mga
Copy link
Member

Superseded by #78119. Thanks for the contribution!

@akien-mga akien-mga closed this Mar 5, 2024
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Mar 5, 2024
@akien-mga akien-mga removed this from the 4.x milestone Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "Attach Node Script" dialog should suggest snake_case file names, consistent with the documentation
10 participants