Skip to content

Conversation

@jfuss
Copy link
Contributor

@jfuss jfuss commented Jan 21, 2020

Issue #, if available:
N/A

Description of changes:
I noticed that the version of Lambda Builders was the whole file. This gets tracked back to when we added

# new regex
>>> re.search(r"__version__ = \"([^'\"]+)", content).group(1)
'0.6.0'

# old regex
>>> re.search(r"__version__ = \"([^']+)\"", content).group(1)
'0.6.0"\nRPC_PROTOCOL_VERSION = "0.3'

This did not happen before because the __version__ was a string in single quotes and RPC_PROTOCOL_VERSION was double quotes. 557214b#diff-eeb75f3f96c23565c44aae048349b2a3 Our regex at that time, assumed only version was the only thing in single quotes in the file (aws_lambda_builders/__init__.py). When we updated to standardize strings to be all double quotes with black, I update this regex assuming the regex was already taking into account many things in the file. This PR fixes that assumption so that the version is only the __version__ value.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

def read_version():
content = read(os.path.join(os.path.dirname(__file__), "aws_lambda_builders", "__init__.py"))
return re.search(r"__version__ = \"([^']+)\"", content).group(1)
return re.search(r"__version__ = \"([^'\"]+)", content).group(1)
Copy link

Choose a reason for hiding this comment

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

Curious to know why we are using regex instead of simply importing the module and read value from variable? Will that not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, importing runs the risk of side affects happening or further importing of modules that are not yet installed. There are more details here: https://packaging.python.org/guides/single-sourcing-package-version/

Copy link

Choose a reason for hiding this comment

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

Cool, I do like approach 3 and 4 in that page, however that is not in scope of this CR :)

@jfuss jfuss merged commit 8c9d476 into aws:develop Jan 21, 2020
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