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

GDScript: Add exclude_addons_exceptions project setting #93889

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Jul 3, 2024

@dalexeev dalexeev added this to the 4.4 milestone Jul 3, 2024
@dalexeev dalexeev requested review from a team as code owners July 3, 2024 09:56
@Mickeon
Copy link
Contributor

Mickeon commented Jul 3, 2024

Bit of an iffy spot to use raw file paths to the .cfg file, especially since the addons folder is hardcoded.

I get we are extremely limited when it comes to UI design here, but this kind of setting would make a whole lot more sense if it was available in the Addons tab as a toggle for each addon, instead. Problem is, this is a bit too specific to deserve a special column, and hiding it behind an "Others" secion would make it too obscure.

I think it would feel a whole lot more natural, beneficial, and encompassing to have both an exclusion list (blacklist) and inclusion list (whitelist, or exceptions as by this PR) of folder/file paths where warnings can apply.
By default, the exclusion list should include the addons folder, and the inclusion list should be empty.

@dalexeev
Copy link
Member Author

dalexeev commented Jul 3, 2024

@Mickeon I thought about both options and initially implemented the Debug column in the Plugins tab. But after some thought, I decided that this was an overcomplication.

You have only two types of code in your project: your own (including some plugins) and third-party. For your own code, you should stick to a consistent style and warning configuration. If you want to suppress some warnings at the individual file level, there is @warning_ignore and #76020, which I hope will eventually be merged.

The option of excluding arbitrary folders or .gitignore-like config would be less efficient. This can be critical since warnings like UNTYPED_DECLARATION are checked quite often.

@dalexeev dalexeev force-pushed the gds-exclude-addons-exceptions-setting branch from 23b7636 to a6211d0 Compare July 3, 2024 11:21
@HolonProduction
Copy link
Member

From a UX perspective, plugins created through the create plugin wizard should be added to this setting automatically, since they are obviously code of the current user.

Also I'd only show the note in the configuration dialog, when the plugin is not on the list, otherwise users might make an unnecessary trip to the settings just to find the plugin already configured.

@dalexeev
Copy link
Member Author

dalexeev commented Jul 4, 2024

@HolonProduction Thanks for the comment! I thought about this option, but there are a few questions we should discuss first:

  1. Is it fine that the addon editor changes the GDScript settings? Or should we implement the method in the GDScript module and call it in the addon editor using conditional compilation?
  2. Should we provide a UI for editing GDScript settings in the addon editor for existing plugins or just add the exception when creating a plugin?
  3. If the user renames/moves the plugin through the File System dock, should we update the GDScript settings?

@HolonProduction
Copy link
Member

HolonProduction commented Jul 4, 2024

  1. I think, if it is fine to check the language name against the hardcoded GDScript language name, it should be fine to meddle with the GDScript settings, checking if a hardcoded settings path exists. But maybe someone from the editor team has more insight on the guidelines for interaction with modules.

  2. On one hand side I think it wouldn't hurt to have the option in both places, on the other I think it doesn't really fit with the other stuff in this dialog (properties of the plugin vs. properties of the project). Haven't really made my mind up about this 😅

  3. That sounds reasonable and should be possible entirely in the gdscript module. But given that the project settings seem to handle moving stuff with settings that are just a single file path, I'd rather consider a change to make the project settings handle lists of file paths as well.

@Jack-023
Copy link

Jack-023 commented Jul 8, 2024

Hi I opened one of the feature requests that is listed as being closed by this change. If I am understanding correctly, this change is actually the inverse of what I was requesting. I would like to be able to disable warnings for specific directories outside of res://addons, as I am using imjp94/gd-plug to manage addons which keeps a local copy of the git repo of each plugin in res://.plugged. This would be the blacklist that @Mickeon was describing but doesn't actually seem to be implemented in this PR.

@dalexeev
Copy link
Member Author

dalexeev commented Jul 8, 2024

I would like to be able to disable warnings for specific directories outside of res://addons

@Jack-023 This PR applies to addons only. In my opinion, it is not worth adding excessive configurability to arbitrary folders. You only have two script types in a project:

  • for your own or project-specific code, you should use a consistent style and warning configuration;
  • for third-party or project-agnostic code, you should just ignore all warnings.

So you should always place third-party code in res://addons. The only problem is that sometimes you need to place your own code in res://addons. With this PR you can add exceptions and mark some folders in res://addons as your own code. Note that this PR adds a warning to the exclude_addons documentation that it is not recommended to disable the setting. We can even mark it as deprecated.

If you need to disable warnings for individual files, then there are other proposals:

@HolonProduction
Copy link
Member

HolonProduction commented Jul 8, 2024

as I am using imjp94/gd-plug to manage addons which keeps a local copy of the git repo of each plugin in res://.plugged.

This seems to be a known issue of the addon: imjp94/gd-plug#32

As a workaround you can change the path to res://addons/plug-gd/.plugged. It seems to be easily configurable through a const variable in the plug.gd script.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 10, 2024

Wouldn't it be enough to enable warnings if the addon script is opened in script editor? This way you can see warnings and don't need to configure anything.

@dalexeev
Copy link
Member Author

I think not. First, for your own code, you would probably like to see all the warnings in the Debugger, even if the file is not open in the editor. Just because a plugin didn't have errors/warnings before doesn't mean they didn't appear after a Godot version update or other changes in the project.

Second, this would mean that for addons, warnings with Error level should be treated as Warn, which is quite confusing (especially for warnings like NATIVE_METHOD_OVERRIDE). Otherwise, you may get different behavior depending on whether the script is open in the editor or not.

@HolonProduction
Copy link
Member

I just had an idea:

You only have two script types in a project:

  • for your own or project-specific code, you should use a consistent style and warning configuration;
  • for third-party or project-agnostic code, you should just ignore all warnings.

The whole solution is based on this assumption, so I think the setting should reflect that and be named in a way that doesn't tie it to warnings.

For example, based on godotengine/godot-proposals#10174 I had the idea to redirect symbol lookup to the documentation page instead of the code, if the script was located in addons. But while convenient for users it would be annoying for addon devs, and comes down to the same problem, I want the script for my own code and the documentation for external code.

We could use the setting for such use cases as well. But that would be very confusing with the current naming.

@pineapplemachine
Copy link

Ideally, I think it should be possible for plugins to define their own GDScript settings independent of the overall project. But short of that, having just this improvement for now would be extremely helpful to me, since my current project involves a bespoke EditorPlugin, I rely on typing errors to catch coding mistakes, and I also rely on addons written by others that do not all use typing comprehensively, meaning I am stuck in a place where including addons for GDScript warnings and errors helpfully shows them for my own plugin but unhelpfully causes other plugins to fail to load.

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.

Project settings: add Debug > GDScript > Warnings parameter to include/exclude specific add-ons
6 participants