Support reading annotated variables. #102
Conversation
There was a problem hiding this comment.
Thanks for the contrib.
In my opinion, this change attempts to do too many things, specifically:
- Refactor _read to return None instead of raising an exception.
- Add support for reading annotated variables.
- Alters the behavior if multiple variables are assigned.
- Re-writes _read from a functional approach to an imperative one.
Moreover, it's unclear to me why supporting annotated variables is desirable. Don't the type checkers automatically infer types for static literal assignments? I'm slightly inclined to discourage type annotations on such assignments.
i.e. I find it much more appealing to read and write:
__requires__ = ['foo']
Than
__requires__:list[str]=['foo']
Often I would simply request a clarification of the motivation for the change in the PR, but since this PR is already doing too much, I'm going to decline it and instead request that you file an issue describing your motivation for adding support for type annotations. Let's determine first if it's desirable to have such support. Additionally, if you want to change the behavior around duplicate (ambiguous) variables or alter the logic for handling exceptions, let's tackle those concerns separately.
Thanks!
| 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 | ||
| else: | ||
| continue | ||
| if isinstance(target, ast.Name) and target.id == var_name: | ||
| if code: | ||
| code = None | ||
| break | ||
| code = node.value | ||
| return ast.literal_eval(code) if code else default |
There was a problem hiding this comment.
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).
| if code: | ||
| code = None | ||
| break |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see. But why would it be a different outcome?
There was a problem hiding this comment.
I'm suggesting the same outcome, just a different approach.
This change alters a private method that simplifies its usage in the only method that actually uses it.
It preserves that behavior. Or am I wrong the functional version handled multiple variables differently?
Yeah, that was opinionated. I assumed imperative approach will be simpler than a very huge list comprehension with error-prone unpacking, some exceptions of which seemed to be caught "by luck" when used by |
Yes, however there is a not-so-rare use case to annotate them as |
Yes, you're right. I'd mis-read your description and didn't realize you were merely preserving the existing behavior. The additional test had me thinking about it as a new change, but now I see it's not. Thanks for clarifying.
Yes, that's kinda true about being caught "by luck". I'm addressing that in #106. On the other hand, I do often code non-defensively, letting edge cases be handled by exceptions and only addressing concerns if they reveal themselves as practical concerns. The error-prone unpacking was there intentionally, meaning to match only if the variable appeared once. That approach keeps the method signature very clean (no extra parameters and a non-optional return type). I've updated the docstring to reflect that intent. |
|
Thanks for the explanation! I'll try to use a different approach here. Filing an issue on supporting reading annotated variables first. |
| elif isinstance(node, ast.AnnAssign): | ||
| target = node.target |
There was a problem hiding this comment.
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
Great! In c9a7094, I've started thinking about how I might refactor the code to support such a change. |
Adds support for reading
__requires__and__index_url__variables if they are annotated and preserves the behavior of recognizing no dependencies in case of ambiguity (multiple definitions of the same variable in the same indent level).Since this change, the
DepsReader._readmethod is safe and requires no outer exception handling when used.