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.

Sign up for GitHub

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

Jump to bottom

Support reading annotated variables. #102

Closed
wants to merge 3 commits into from
Closed

Support reading annotated variables. #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ad4906
Support reading annotated variables.
bswck Jun 19, 2024
bfe4e39
Add news entry.
bswck Jun 19, 2024
4b047b9
Add additional dep to multiple annotated deps test.
bswck Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions newsfragments/102.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support reading annotated variables (e.g. `__requires__: list[str] = ["jaraco.functools"]`).
31 changes: 17 additions & 14 deletions pip_run/scripts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import abc
import ast
import contextlib
import itertools
import json
import pathlib
Expand Down Expand Up @@ -151,24 +150,28 @@ def read_python(self):
>>> DepsReader(r"__requires__='foo\nbar\n#baz'").read()
['foo', 'bar']
"""
raw_reqs = suppress(ValueError)(self._read)('__requires__') or []
raw_reqs = self._read('__requires__', default=())
reqs_items = jaraco.text.yield_lines(raw_reqs)
deps = Dependencies.load(reqs_items)
with contextlib.suppress(Exception):
deps.index_url = self._read('__index_url__')
deps.index_url = self._read('__index_url__')
return deps

def _read(self, var_name):
def _read(self, var_name, default=None):
mod = ast.parse(self.script)
(node,) = (
node
for node in mod.body
if isinstance(node, ast.Assign)
and len(node.targets) == 1
and isinstance(node.targets[0], ast.Name)
and node.targets[0].id == var_name
)
return ast.literal_eval(node.value)
code = None
for node in mod.body:
if isinstance(node, ast.Assign) and len(node.targets) == 1:
(target,) = node.targets
elif isinstance(node, ast.AnnAssign):
target = node.target
Comment on lines +165 to +166
Copy link
Owner

@jaraco jaraco Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, interesting - TIL an AnnAssign can never be part of a multi-target assignment:

 pip-run main @ py -q
>>> x:str, y:str = '3', '4'
  File "<unknown>", line 1
    x:str, y:str = '3', '4'
         ^
SyntaxError: invalid syntax

else:
continue
if isinstance(target, ast.Name) and target.id == var_name:
if code:
code = None
break
Comment on lines +170 to +172
Copy link
Owner

Choose a reason for hiding this comment

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

It took me a while to figure out that this block here is for the duplication detection. It's nested several levels deep and alters other state variables, entangling it with the other concerns in this method. Much better would be to have a _read_all (or similar) that generates all of the targets and have that function react on the condition of "more than one", addressing that concern at a higher level rather than tweaking the internal state to simulate a different outcome.

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 see. But why would it be a different outcome?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm suggesting the same outcome, just a different approach.

code = node.value
return ast.literal_eval(code) if code else default
Comment on lines +161 to +174
Copy link
Owner

Choose a reason for hiding this comment

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

This rewrite of the function changes the mccabe cyclomatic complexity from 1 to 6, indicating that the new code is substantially more complex. This change introduces two new state variables (target and code, both of which are assigned in multiple places and whose values affect the control flow. My preference is to keep the control flow as simple as possible (which is why you'll find very few for or if statements in the pip-run code base).



class SourceDepsReader(DepsReader):
Expand Down
25 changes: 25 additions & 0 deletions tests/test_scripts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,31 @@ def test_single_dep(self):
)
assert scripts.DepsReader(script).read() == ['foo']

def test_single_annotated_dep(self):
script = textwrap.dedent(
"""
__requires__:str='foo'
"""
)
assert scripts.DepsReader(script).read() == ['foo']

def test_multiple_annotated_deps(self):
script = textwrap.dedent(
"""
__requires__:list[str]=['foo', 'bar']
"""
)
assert scripts.DepsReader(script).read() == ['foo', 'bar']

def test_skips_on_ambiguity(self):
script = textwrap.dedent(
"""
__requires__:list[str]=['foo']
__requires__='bar'
"""
)
assert scripts.DepsReader(script).read() == []

def test_index_url(self):
script = textwrap.dedent(
"""
Expand Down
Loading