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

Allow to extends constant variable(master) #12507

Closed
wants to merge 1 commit into from

Conversation

sanikoyes
Copy link
Contributor

this pr same to : #12506

StringName identifier = tokenizer->get_token_identifier();
p_class->extends_class.push_back(identifier);
}
break;
Copy link
Member

Choose a reason for hiding this comment

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

It looks like clang-format doesn't like the indentation style you used: https://travis-ci.org/godotengine/godot/jobs/294820735

@akien-mga
Copy link
Member

Pre-approved in PR review meeting, will merge tomorrow once 3.0 alpha2 is released, to avoid potential regressions.

if (base_class->subclasses.has(subclass)) {

base_class = base_class->subclasses[subclass];
} else if (base_class->constants.has(subclass)) {

Ref<GDScript> base = base_class->constants[subclass];
Copy link
Contributor

Choose a reason for hiding this comment

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

You are overloading your outer scope variable String base here and I don't think it's a good practice. Try using some different name, for example Ref<GDScript> new_base_class would be better imho.

@akien-mga
Copy link
Member

Sorry for the delay - I've done a refactoring of the gdscript module in #12969 so there are now merge conflicts to resolve. As the file names changed and all GD* classes are now prefixed with GDScript, the faster solution might be to reimplement your changes on top of a clean branch and force push it to override this one.

@akien-mga
Copy link
Member

Merged manually as fb801d4 to fix the merge conflicts (and take @Marqin's review into account).

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.

3 participants