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

Fix POT generation failing on some scripts with class_name. #91963

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

Conversation

EIREXE
Copy link
Contributor

@EIREXE EIREXE commented May 14, 2024

Previously, it would fail with the error "Class "" hides a global script class"

Original error at line:

https://github.com/godotengine/godot/blob/master/modules/gdscript/gdscript_analyzer.cpp#L364

Previously, it would fail with the error "Class "<class>" hides a global script class"
@EIREXE EIREXE requested a review from a team as a code owner May 14, 2024 22:35
@dalexeev dalexeev added this to the 4.3 milestone May 15, 2024
@dalexeev
Copy link
Member

Could you please provide an MRP to reproduce the bug?

@akien-mga akien-mga changed the title Fix MOT generation failing on some scripts with class_name. Fix POT generation failing on some scripts with class_name. May 15, 2024
@@ -52,10 +52,16 @@ Error GDScriptEditorTranslationParserPlugin::parse_file(const String &p_path, Ve
ids = r_ids;
ids_ctx_plural = r_ids_ctx_plural;
Ref<GDScript> gdscript = loaded_res;
ERR_FAIL_COND_V_MSG(!gdscript->is_valid(), ERR_FILE_CANT_OPEN, "Failed to load GDScript.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be:

Suggested change
ERR_FAIL_COND_V_MSG(!gdscript->is_valid(), ERR_FILE_CANT_OPEN, "Failed to load GDScript.");
ERR_FAIL_COND_V_MSG(!gdscript.is_valid(), ERR_FILE_CANT_OPEN, "Failed to load GDScript.");

I.e.

Suggested change
ERR_FAIL_COND_V_MSG(!gdscript->is_valid(), ERR_FILE_CANT_OPEN, "Failed to load GDScript.");
ERR_FAIL_COND_V_MSG(gdscript.is_null(), ERR_FILE_CANT_OPEN, "Failed to load GDScript.");

Or:

Suggested change
ERR_FAIL_COND_V_MSG(!gdscript->is_valid(), ERR_FILE_CANT_OPEN, "Failed to load GDScript.");
ERR_FAIL_COND_V_MSG(gdscript.is_null() || !gdscript->is_valid(), ERR_FILE_CANT_OPEN, "Failed to load GDScript.");

Is this check for the script or the resource?

@EIREXE
Copy link
Contributor Author

EIREXE commented May 16, 2024

I gotta admit I rushed this PR a bit too much, i'll try to make a proper MRP

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.

None yet

3 participants