Skip to content

Conversation

@Mysteryem
Copy link
Contributor

What is this fixing or adding?

Replaces the eval in OptionsCreator.py with converting strings to int/bool directly.

eval, while probably safe here, can be a security concern, which could have complicated maintenance and/or refactors.

How was this tested?

I clicked the "Random?" button on numeric, boolean and other options, with extra logging to log the new value of self.options[name] each time, and which conditional branch was being hit.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jan 8, 2026
if self.options[name].isnumeric() or self.options[name] in ("True", "False"):
self.options[name] = eval(self.options[name])
if self.options[name].isnumeric():
self.options[name] = int(self.options[name])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose this isn't a new issue, but .isnumeric() doesn't seem like the check that's actually wanted here. For instance, a Range that's set to a negative value and then toggled and untoggled random will end up being output as a string since something like "-1".isnumeric() gives False, although it should still work if from_any is used. If any int-castable value here should be accepted, that should be done by trying to do it in a try:.

I'd also be concerned about other similar issues when trying to determine type based just on string, although something like a choice called True seems to be spared by being lowercase.

Copy link
Collaborator

@beauxq beauxq left a comment

Choose a reason for hiding this comment

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

I think the isnumeric is a different issue that can be discussed and fixed in a different PR, and should not block this PR.

Copy link
Contributor

@gerbiljames gerbiljames left a comment

Choose a reason for hiding this comment

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

Code makes sense to me, agreed on the isNumeric() change being for another PR

@duckboycool duckboycool added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jan 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

affects: core Issues/PRs that touch core and may need additional validation. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants