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

Rework javascript getProperty API to handle missing properties better #4900

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

fishface60
Copy link
Contributor

@fishface60 fishface60 commented Sep 8, 2024

Identify the Bug or Feature request

closes #3028

Description of the Change

This changes the javascript getProperty function to look up the value in the campaign property type defaults if it's missing, and return null instead of "null" if you don't have permission to get that token's properties or the campaign doesn't have that property either, for improved ergonomics of null-handling in javascript.

This also adds getRawProperty which returns null if it's not set, and getEvaluatedProperty which may evaluate the value as a macro.

Possible Drawbacks

If users of getProperty were checking for the value being missing with something like:

let value = token.getProperty(name);
if (value == "null") {
    value = "10";
}

then this will now end up with value being null.

getProperty is the most desirable name for the most common operation. It's possible that most campaigns use properties that should be evaluated so should use the shorter name, and the form of getProperty that only falls back to the default value should be called something like getUnevaluatedProperty or getDefaultedProperty.

Release Notes

  • Made token.getProperty return null instead of the string "null" when properties are missing or not permitted to be read.
  • Make token.getProperty return the default value for the token type if the value was not set on the token.
  • Added token.getRawProperty for uses that shouldn't fall back to defaults and token.getEvaluatedProperty for uses that should use calculated defaults.

This change is Reviewable

@fishface60
Copy link
Contributor Author

I'll have a poke at CI failures this evening.

@cwisniew
Copy link
Member

cwisniew commented Sep 9, 2024

I'll have a poke at CI failures this evening.

They are formatting errors you can fix with
./gradlew spotlessApply (or .\grqladlew.bat spotlessApply of using windows)

@fishface60
Copy link
Contributor Author

I'll have a poke at CI failures this evening.

They are formatting errors you can fix with ./gradlew spotlessApply (or .\grqladlew.bat spotlessApply of using windows)

I even told myself "You should run that before submitting". I blame staying up until 11PM because the current token behaviour had been bothering me for long enough and doing bindings token status bars brought me close enough to the code to feel like I could do something about it.

null is returned if the property does not exist, does not have a default
or you aren't permitted to see the property.
Previously you could tell that the property was unset by if it returned
the string "null", so getRawProperty gives that feature back
but returning a null so you can use ?? to default it yourself.

getEvaluatedProperty adds the ability to handle calculated defaults.
getProperty was not made to use getEvaluatedProperty
because a javascript function that knows the campaign doesn't use
calculated defaults could use it to save on performance,
but also that switching to evaluating by default is more likely to be a
breaking change.

The code for handling a missing parameter in earlier versions is awkward
so it's likely that code that uses getProperty is used in campaigns
where there is no default value to be reset to and all properties are
explicitly set so doesn't trip the awkward behaviour.

However, if this code contained objects that could be mis-parsed as
macros beginning to use macros by default would be a breaking change.
@cwisniew cwisniew added this pull request to the merge queue Sep 12, 2024
Merged via the queue into RPTools:develop with commit aed33aa Sep 12, 2024
4 checks passed
@cwisniew cwisniew added the bug label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[1.10 JavaScript] Token.getProperty does not return default value and does not process calculated properties.
2 participants