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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow turning GDScript warning categories into errors individually (instead of a global setting) #3531

Closed
Calinou opened this issue Nov 11, 2021 · 6 comments 路 Fixed by godotengine/godot#59943

Comments

@Calinou
Copy link
Member

Calinou commented Nov 11, 2021

Related to #3284.

Describe the project you are working on

The Godot editor 馃檪

Describe the problem or limitation you are having in your project

It's not currently possible to turn a single GDScript warning category into an error. Instead, you can only enable warning-to-error conversion for all enabled warnings in the Project Settings.

See godotengine/godot#54883 for context.

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

Allow turning GDScript warning categories into errors individually, instead of only having a global setting that enables all warning-to-error conversions. This allows for greater control, especially when only some warnings can be turned into errors without harming productivity too much.

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

Change boolean warning properties to an enum with 3 values:

  • Ignore (default for the currently disabled warnings)
  • Warning (default for the currently enabled warnings)
  • Error

Then remove the warnings-to-errors project setting, as it won't be needed anymore.

To improve the robustness of GDScript code written by users, we can consider making some warnings errors by default. However, this is likely better discussed in a separate proposal.

This configuration behavior is consistent with linters such as ESLint.

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

No, as the GDScript parser is implemented in C++ and its behavior can't be overridden with extensions.

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

See above.

@Xrayez
Copy link
Contributor

Xrayez commented Nov 14, 2021

I would definitely use this feature for treating #3463 an a error, among other warnings like Shadowed variable.

@hilfazer
Copy link

To improve the robustness of GDScript code written by users, we can consider making some warnings errors by default. However, this is likely better discussed in a separate proposal.

Good idea. I'm against this particular suggestion but not against this proposal in general.

@ghost
Copy link

ghost commented Dec 18, 2021

You could do something like this

image

@Calinou
Copy link
Member Author

Calinou commented Dec 18, 2021

You could do something like this

image

This would make the inspector code more complicated for little benefit. It would also allow setting up conflicting options (warning disabled but "as errors" enabled). I think using enum properties is the way to go here.

@ghost
Copy link

ghost commented Dec 18, 2021

Yeah maybe is a bit much you are right. Enum properties I assume would show up for some of the Warnings in place of a checkbox?

@Calinou
Copy link
Member Author

Calinou commented Dec 18, 2021

Enum properties I assume would show up for some of the Warnings in place of a checkbox?

Yes, enum properties would replace all the existing checkboxes in the GDScript Warnings section of the project settings.

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.

4 participants