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

Util string_to_list with braces #1361

Merged
merged 3 commits into from May 19, 2019

Conversation

@avanwinkle
Copy link
Collaborator

commented May 8, 2019

The changes in #1327 caused new evaluations of Util.string_to_list that can result in the splitting of conditional events when merging dictionaries.

This PR extends the logic of string_to_list to maintain spaces within braces when it splits, resulting in the expected behavior

>>> Util.string_to_list("event1 event2{not some_condition} event3")
["event1", "event2{not some_condition}", "event3"]

Because the regex is more computionally demanding than the native String.replace().split(), the new logic is only performed if the string has a brace. Otherwise, the old logic is used.

@jabdoa2

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

Wow where did I break that? Thanks for the fix!

new_list = re.findall(r'([\w|-]+?\{.*?\}|[\w|-]+)', string)
else:
# Convert commas to spaces, then split the string into a list
new_list = string.replace(",", " ").split()

This comment has been minimized.

Copy link
@jabdoa2

jabdoa2 May 10, 2019

Collaborator

I guess whitespace and/or comma is also not such a good idea, right? We should convert this into something more explicit (not necessarily now)

This comment has been minimized.

Copy link
@avanwinkle

avanwinkle May 12, 2019

Author Collaborator

It feels like an "it was good enough at the time" decisions, and generally speaking it's sufficient. I wouldn't want to change anything that would break existing configs, although accepting both commas and spaces is more complex than doing strict space separation.

@avanwinkle

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

Honestly @jabdoa2 I couldn't find exactly where it was broken. I tracked the bug based on the logs throwing for unknown slides, widgets, et cetera with names like slide_singleplayer{ball==3 and that led me to the spaces in conditionals.

I then started working back through commits and found that the event player merge was what broke it, but I couldn't find where. Just made a fix instead :)

@avanwinkle avanwinkle merged commit b625a93 into missionpinball:dev May 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

avanwinkle pushed a commit to avanwinkle/mpf that referenced this pull request May 19, 2019

Anthony van Winkle Anthony van Winkle
Merge branch 'dev' into machine-settings-bugfix
* dev:
  Util string_to_list with braces (missionpinball#1361)

@avanwinkle avanwinkle deleted the avanwinkle:util-line-split-with-braces branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.