search

jaraco / pip-run

pip-run - dynamic dependency loader for Python
MIT License
130 stars 19 forks source link

Support reading annotated variables. #102

Closed bswck closed 1 week ago

bswck commented 1 week ago

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._read method is safe and requires no outer exception handling when used.

bswck commented 1 week ago
  • Refactor _read to return None instead of raising an exception.

This change alters a private method that simplifies its usage in the only method that actually uses it. Exception handling inside _read makes it a universally reusable method.

  • Add support for reading annotated variables.
  • Alters the behavior if multiple variables are assigned.

It preserves that behavior. Or am I wrong the functional version handled multiple variables differently?

  • Re-writes _read from a functional approach to an imperative one.

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 read_python.

bswck commented 1 week ago

Don't the type checkers automatically infer types for static literal assignments? I'm slightly inclined to discourage type annotations on such assignments.

Yes, however there is a not-so-rare use case to annotate them as Final: https://github.com/search?q=%22__all__%3A+Final%22&type=code

jaraco commented 1 week ago

It preserves that behavior. Or am I wrong the functional version handled multiple variables differently?

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.

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 read_python.

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.

bswck commented 1 week ago

Thanks for the explanation! I'll try to use a different approach here. Filing an issue on supporting reading annotated variables first.

jaraco commented 1 week ago

Thanks for the explanation! I'll try to use a different approach here. Filing an issue on supporting reading annotated variables first.

Great! In c9a7094, I've started thinking about how I might refactor the code to support such a change.

bswck commented 1 week ago

Thanks for the explanation! I'll try to use a different approach here. Filing an issue on supporting reading annotated variables first.

Great! In c9a7094, I've started thinking about how I might refactor the code to support such a change.

Oh, does that mean #107 has your thumbs up?