Skip to content

Conversation

@nicopop
Copy link
Contributor

@nicopop nicopop commented Feb 1, 2026

Since I have seen some people that still get identifier errors while using options, im making this PR to force the options names to be valid as python detects it

reminder that option display_name stays to whatever invalid name they defined in options.json/category.json this function just make sure its valid as a key

@nicopop nicopop self-assigned this Feb 1, 2026
@nicopop
Copy link
Contributor Author

nicopop commented Feb 3, 2026

Ill probably need to add support for accented characters since those appear to be valid for identifiers.
A slightly disgusting fix could be to check each non numeric char in the string through str.isidentifier()

@nicopop
Copy link
Contributor Author

nicopop commented Feb 4, 2026

I tested and using str.isidentifier() against every character is actually pretty free (on the ms taken side)
it might start to influence timing if devs try to use really long options names but if they make sure its already valid then the first check skip all the rest of the code

…n every char so weird non identifier decimal are still caught
@nicopop
Copy link
Contributor Author

nicopop commented Feb 11, 2026

here's what I used to test the timing:
image
+ a call in hooks/world.py: after_fill_slot_data logging.info(f"identifier conversion took {get_valid_ident_timing():.4f} seconds total")
at worst I got a 0.0035 seconds but its usually around 0.0010
if we want to reduce the time spent even more we could slap a @cache (from functools) on it and then the average is smaller than 0.0000 seconds
but at this point I think its fast enough and compatible with as many character we can

Copy link
Contributor

@silasary silasary left a comment

Choose a reason for hiding this comment

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

Yeah, I don't think it gets called enough to justify @cache usage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants