imjp94 / gd-plug

Minimal plugin manager for Godot
MIT License
215 stars 15 forks source link

Inner class name conflict with other plugins #4

Closed lihop closed 3 years ago

lihop commented 3 years ago

After installing and enabling the ThreadPool plugin with the following plug.gd:

extends "res://addons/gd-plug/plug.gd"

func _plugging():
    plug("zmarcos/godothreadpool")

Future invocations of the plug.gd script fail with the error messages:

ERROR: get_global_class_path: Condition "!global_classes.has(p_class)" is true. Returned: String()
   At: core/script_language.cpp:239.
SCRIPT ERROR: GDScript::reload: Parse Error: Can't override name of the unique global class "ThreadPool". It already exists at: 
   At: res://addons/gd-plug/plug.gd:832.
ERROR: reload: Method failed. Returning: ERR_PARSE_ERROR
   At: modules/gdscript/gdscript.cpp:583.
SCRIPT ERROR: GDScript::reload: Parse Error: Script isn't fully loaded (cyclic preload?): res://addons/gd-plug/plug.gd
   At: res://plug.gd:1.
ERROR: reload: Method failed. Returning: ERR_PARSE_ERROR
   At: modules/gdscript/gdscript.cpp:583.

This is because the ThreadPool plugin declares the unique global class here:

class_name ThreadPool

which conflicts with gd-plug's inner class: https://github.com/imjp94/gd-plug/blob/83cd8f5409d98a54e0f2e02327bd9b869c2706a6/addons/gd-plug/plug.gd#L832

I can also see the potential for conflict with gd-plug's other inner classes. Logger with some logging related plugin, and (maybe less likely) GitExecutable with some git related plugin.

More widely, this is related to github proposal #1566 Implement namespaces to avoid collisions in third-party add-ons. A suggested workaround there is to add a prefix to class names.

Some ideas (assuming these inner classes are for private use only and not part of the pubilc gd-plug API):

imjp94 commented 3 years ago

Now there's another reason for me to avoid class_name...

Personally, I prefer the approach of adding underscore prefix. As it make sense even if the issue(Implement namespaces to avoid collisions in third-party add-ons #1566) is fixed in the future. And as mentioned that this approach doesn't fully mitigate the conflict, but I think the chance is so low that it is neglectable.

Thanks for the bug report!