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

Avoid KeyError on variable reuse #64

Merged
merged 2 commits into from
Aug 5, 2021
Merged

Conversation

alf239
Copy link
Contributor

@alf239 alf239 commented Jun 24, 2021

When there are multiple files in target, and they happen to share a version variable, it is only replaced in the first file in the list, as ReplacementHandler throws KeyError trying to remove the field from the missing list.

Verified

This commit was signed with the committer’s verified signature.
crazy-max CrazyMax
@@ -30,5 +30,6 @@ def __call__(self, match):
# of the line just after the last non-whitespace character
# e.g. blah=\n --> blah=text\n
start = end = len(original.rstrip())
self.missing.remove(key)
if key in self.missing:
self.missing.remove(key)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The gist of it.

[AutoVersionConfig]
CONFIG_NAME = 'example'
PRERELEASE_TOKEN = 'dev'
targets = ["example.py", "example2.py"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference to example.toml: two targets

@@ -0,0 +1,6 @@
LOCK = False
RELEASE = True
VERSION = "19.99.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Literally the same file as example.py - we just need to hit at least one variable that was already substituted elsewhere.

@alf239
Copy link
Contributor Author

alf239 commented Jun 24, 2021

A more elaborated approach would be to key the variables by file, so that variables with the same name, but in different files would be handled as separate substitutions; that would change the user-facing API though, so I opted for a simpler solution.

@@ -1,2 +1,3 @@
toml
semver~=2.13
six
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm slightly confused with this one: you clearly use six in the sources; still, it's not in the requirements.txt. Maybe I'm just missing some pythonesque feature, please forgive me then, I'm happy to revert.

with open("example2.py", "r") as f:
second_file = f.read()
self.assertEqual(second_file, '''LOCK = False
RELEASE = True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would I readlines(), the assert would look less ugly; but then my python was glueing up two lines together, faithfully keeping \n in the middle, and I decided that the life's too short.

@alexl0gan alexl0gan requested a review from acabarbaye June 25, 2021 07:58

Verified

This commit was signed with the committer’s verified signature.
crazy-max CrazyMax
@acabarbaye acabarbaye merged commit 10167be into ARMmbed:master Aug 5, 2021
@acabarbaye
Copy link
Collaborator

@alf239 Thank you for your contribution. Your change has now been integrated and released onto PyPI.

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.

None yet

2 participants