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 GDScript parser suggestions (for situations where warnings are not suitable) #4310

Open
jordigcs opened this issue Mar 30, 2022 · 7 comments

Comments

@jordigcs
Copy link

Describe the project you are working on

Godot Script Editor improvements

Describe the problem or limitation you are having in your project

#3284 would be better implemented with a warning-like suggestion system that can be dismissed.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Adding a suggestion system will help beginner programmers write more readable and concise code, and allow for advanced programmers to enforce a certain style of code with optional suggestions that can be enabled. (Such as #3284)

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

For example, when constants don't use CONSTANT_CASE:
image
image

If this enhancement will not be used often, can it be worked around with a few lines of script?

No

Is there a reason why this should be core and not an add-on in the asset library?

It can be better integrated and used by future features if it's core editor functionality.

@jordigcs jordigcs changed the title Add the ability for the GDScript parser to push "suggestions" Add GDScript parser suggestions Mar 30, 2022
@Calinou
Copy link
Member

Calinou commented Mar 30, 2022

Related to #3630.

I think having additional suggestions is a good idea nonetheless, as it encourages beginners to follow the GDScript style guide.

This would effectively add a 3rd level of reporting to the GDScript parser ("error", "warning", "suggestion"), but many IDEs such as JetBrains already do something similar in their code inspections.

To improve run-time performance and reduce binary size, the suggestion code should only be enabled in editor builds. Unlike warnings, it probably shouldn't cause prints to be displayed in the Output or Debugger > Errors panels either. Therefore, each reporting level would be available in various build types as follows:

  • Error in editor, debug template and release template builds.
  • Warning in editor and debug template builds only.
  • Suggestion in editor builds only.

PS: I prefer calling "macro case" UPPER_SNAKE_CASE, especially since GDScript doesn't have macros.

@jordigcs
Copy link
Author

jordigcs commented Mar 31, 2022

Related to #3630.

I think having additional suggestions is a good idea nonetheless, as it encourages beginners to follow the GDScript style guide.

This would effectively add a 3rd level of reporting to the GDScript parser ("error", "warning", "suggestion"), but many IDEs such as JetBrains already do something similar in their code inspections.

To improve run-time performance and reduce binary size, the suggestion code should only be enabled in editor builds. Unlike warnings, it probably shouldn't cause prints to be displayed in the Output or Debugger > Errors panels either. Therefore, each reporting level would be available in various build types as follows:

  • Error in editor, debug template and release template builds.
  • Warning in editor and debug template builds only.
  • Suggestion in editor builds only.

PS: I prefer calling "macro case" UPPER_SNAKE_CASE, especially since GDScript doesn't have macros.

I'm implementing this proposal, suggestions are surrounded in #ifdef TOOLS_ENABLED to make sure they're only available in editor builds. Right now all the suggestions I have are based on the GDScript Style Guide:

image

Suggestions are just warnings with is_suggestion set to true, they show up in the warnings panel like regular warnings. When there's no warnings, but there are suggestions, the warning button changes its style:

image

image

Warnings have priority over suggestions in the status bar. Should this be the default behavior?

@Calinou
Copy link
Member

Calinou commented Mar 31, 2022

Great work so far!

Warnings have priority over suggestions in the status bar. Should this be the default behavior?

Yes, as warnings are more important to resolve first.

@jordigcs
Copy link
Author

Great work so far!

Warnings have priority over suggestions in the status bar. Should this be the default behavior?

Yes, as warnings are more important to resolve first.

Gotcha, thanks!

@h0lley
Copy link

h0lley commented Mar 31, 2022

and allow for advanced programmers to enforce a certain style of code with optional suggestions that can be enabled.

You are proposing that this should use the same system as warnings do in that users can choose from a list of possible suggestions, yes? However what if a team wants to add a suggestion that is not among the prepared options?

For instance, your example with the casing for constants; what about if a team wants to adopt a style where constants are written like variables? Just disabling the UPPER_SNAKE_CASE suggestion is not enough; they want a suggestion that points out how constants should NOT be written in UPPER_SNAKE_CASE. After all, constants are not what causes harm in our code, variables do, and so there's really no good reason to shout them - it's really just what we are used to. ;)

Mind you this is not at all meant as an argument for a certain style, but as an illustration of how arbitrary many of those suggestions would likely be. Suggestions are all very subjective in nature. Another example; there are many people who have a strong opinion on brackets increasing, not decreasing readability. They may want their team members to be reminded that they ought to use the optional brackets in if statements.

So if a suggestion system were to be implemented, should we perhaps go the extra mile for a more advanced configuration system where rules can be defined that go beyond just a toggle?

@jordigcs
Copy link
Author

jordigcs commented Mar 31, 2022

That's a good point, it would be possible to make the parser suggest a certain naming convention depending on a ProjectSetting, but I think it would make suggestions more verbose than they need to be.

The main reason someone might want to use a suggestion system is to enforce a clean and consistent code writing style. I think allowing developers to go against convention would actually reduce readability, because people that look at the code from an outside point of view would be used to the standardized GDScript style conventions that are already popularized in other languages such as Python.

If we allow the system to be too flexible, we might run into the problem described here.
Specifically this part:

Big and flexible solutions also have an additional drawback which is that, over time, they are rarely flexible enough for all users, who keep requesting more functions added (and making the API and codebase more and more complex).

@Calinou
Copy link
Member

Calinou commented Apr 1, 2022

So if a suggestion system were to be implemented, should we perhaps go the extra mile for a more advanced configuration system where rules can be defined that go beyond just a toggle?

I think suggestions should only cater to the GDScript style guide for simplicity's sake. In my experience, if you intend to do something else, it would be better to use a third-party tool (or nothing). This way, we can keep the suggestion system easy to use and consistent across projects. As long as suggestions can be turned off individually, I don't see a problem with this.

See prettier/prettier#40 as to why you don't want to make things too configurable when it comes to formatting.

@Calinou Calinou changed the title Add GDScript parser suggestions Add GDScript parser suggestions (for situations where warnings are not suitable) Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for Review
Development

No branches or pull requests

3 participants