Skip to content

feat: add interpreter_version_info to py_runtime#1671

Merged
rickeylev merged 4 commits intomainfrom
interpreter_version
Jan 13, 2024
Merged

feat: add interpreter_version_info to py_runtime#1671
rickeylev merged 4 commits intomainfrom
interpreter_version

Conversation

@mattem
Copy link
Copy Markdown
Contributor

@mattem mattem commented Jan 7, 2024

Adds an interpreter_version_info attribute to the py_runtime and associated provider that maps to the sys.version_info values. This allows the version of the interpreter to be known statically, which can be useful for rule sets that depend on the interpreter, and need to build environments / pathing that contain version info (virtualenvs for example).

@mattem mattem force-pushed the interpreter_version branch 4 times, most recently from 245f06c to 238de8e Compare January 7, 2024 15:52
Copy link
Copy Markdown
Member

@f0rmiga f0rmiga left a comment

Choose a reason for hiding this comment

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

I'll let other folks bikeshed if they want.

Copy link
Copy Markdown
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

My main comment is to use a dict/struct instead of list. This makes it easier to know which parts are check for the relevant pieces. Plus we can stash extra implementation-specific parts in there if necessary.

i.e. accept a attr.string_dict on the rule and and convert it to a struct() for the provider. Convert particular keys to their appropriate types (e.g. major/minor/patch to ints, similar to sys.version_info)

Please add tests. You can add them to //tests/py_runtime

@mattem mattem force-pushed the interpreter_version branch from 238de8e to ef00f2b Compare January 8, 2024 17:44
@mattem mattem requested a review from rickeylev January 8, 2024 17:45
@mattem mattem force-pushed the interpreter_version branch from ef00f2b to cee2188 Compare January 8, 2024 17:49
@mattem mattem force-pushed the interpreter_version branch from cee2188 to 0090758 Compare January 9, 2024 00:43
@mattem
Copy link
Copy Markdown
Contributor Author

mattem commented Jan 9, 2024

@rickeylev Can I have another pass on this please?

@rickeylev
Copy link
Copy Markdown
Collaborator

Looks pretty good! Thank you for adding tests.

Can you please add an entry to CHANGELOG.md? Then it's ready to merge.

@mattem mattem force-pushed the interpreter_version branch from 0090758 to 3a92745 Compare January 12, 2024 11:37
@rickeylev rickeylev enabled auto-merge January 13, 2024 06:58
@rickeylev rickeylev added this pull request to the merge queue Jan 13, 2024
Merged via the queue into main with commit 5238141 Jan 13, 2024
@rickeylev rickeylev deleted the interpreter_version branch January 17, 2024 17:12
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.

3 participants