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 some _validate_property not correctly refactored #64553

Merged

Conversation

RedMser
Copy link
Contributor

@RedMser RedMser commented Aug 18, 2022

Some old code was checking for properties that used / for category separation, but now _ is used instead.

Found with regex: property\.name.+\/


Also removed PROPERTY_USAGE_INTERNAL in _validate_property as this flag is only relevant for doctool, which only checks the property definition's usage flags.

Found with regex: property\.usage.+INTERNAL

@RedMser RedMser requested a review from a team as a code owner August 18, 2022 00:58
@Chaosus Chaosus added this to the 4.0 milestone Aug 18, 2022
@RedMser RedMser force-pushed the fix-property-validation-refactor branch from acab1d1 to 8da9f8c Compare August 18, 2022 12:45
@RedMser RedMser requested a review from a team as a code owner August 18, 2022 12:45
@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 18, 2022

is it okay for the property usage to stay "NONE" instead of "NO_EDITOR"?

NO_EDITOR means it's not visible in the Inspector, but I guess in this case we don't want them to be serializable either. PROPERTY_USAGE_INTERNAL in there is likely a misuse though.

If a specific default value must be set regardless, there is also DEFVAL().

The flag only matters for property definition,
but was also used in _validate_property a lot.
@RedMser RedMser force-pushed the fix-property-validation-refactor branch from 8da9f8c to c25b8f2 Compare August 18, 2022 14:25
@RedMser RedMser requested review from a team as code owners August 18, 2022 14:25
@RedMser
Copy link
Contributor Author

RedMser commented Aug 18, 2022

PROPERTY_USAGE_INTERNAL in there is likely a misuse though.

9b8e8b2 split off INTERNAL from NO_EDITOR, so I assume they replaced this everywhere, even in places where it makes no difference.
The flag seems to only influence docs generation, which from what I can tell only reads the property definition's usage flags, so I assume this flag shouldn't be used here.
I removed all uses of INTERNAL inside of _validate_property as a 2nd commit, let me know if that's okay. (Find-and-replaced property\.usage.+INTERNAL regex and confirmed each result by hand)

If a specific default value must be set regardless, there is also DEFVAL().

Just found the API for it, thanks! I re-added it, since it's useful to see in the docs.

@YuriSizov
Copy link
Contributor

I removed all uses of INTERNAL inside of _validate_property as a 2nd commit, let me know if that's okay. (Find-and-replaced property\.usage.+INTERNAL regex and confirmed each result by hand)

I guess it should be okay, because @reduz complained about it the other day.

@akien-mga akien-mga merged commit fdc36ad into godotengine:master Aug 22, 2022
@akien-mga
Copy link
Member

Thanks!

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.

4 participants