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.

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

Support reading annotated variables #107

Open
bswck opened this issue Jun 22, 2024 · 5 comments
Open

Support reading annotated variables #107

bswck opened this issue Jun 22, 2024 · 5 comments
Assignees

Comments

@bswck
Copy link
Contributor

bswck commented Jun 22, 2024

Annotating __requires__/__index_url__ might be discouraged because pip-run only supports literal assignments of __requires__/__index_url__.

This issue aims to advocate for supporting this case nonetheless:

__requires__: Final[str] = "pip-run"

or this case

__requires__: Final[tuple[str, ...]] = ("jaraco.functools", "jaraco.classes")

and so on.

Since __requires__ (and __index_url__ as well) is expected to be an immutable literal defined in one place, I'd suggest:

  • supporting reading annotated variables because of typing.Final which signals: "hey, you shouldn't be reassigning this name __requires__ here"
  • recommending to use immutable type/annotating with an immutable type (i.e. prefer tuples to lists when setting __requires__), e.g. __requires__: Final[Sequence[str]] (not a huge fan because str is itself a Sequence[str], but it correctly matches all types of valid __requires__ values) or __requires__: Final[tuple[str, ...]] which I think best resembles strings' atomicity.

This practice has a bunch of users. Let's check for __all__, which is a common package-related module-level dunder name, typically not expected to change dynamically:

This feature:

  1. is easy to support (it's basically just one additional AST node with even simpler interface and a few small extra tests),
  2. increases the compatibility with all Python codebases.

Moreover, it might increase general type safety knowing that __requires__ can either be a collection of strings or a single string, treated atomically.

I do think this feature is in pip-run's philosophy and makes its usability wider at not much of a cost.

@jaraco
Copy link
Owner

jaraco commented Jun 22, 2024

Nice. Thanks for documenting the motivation and especially finding prior art. I'd not thought to look at __all__. I particularly appreciate the ability to declare the variable Final, which apparently can be done without overspecifying the type.

I do see the value in supporting valid syntax. Especially since this library is being shared for more than just the pip-run cli.

On the other hand, there's lots of valid Python syntax that this construct doesn't support, such as mutation (__requires__.append('another')) or multi-value assignment (__requires__, __index_url__ = ...) or non-literal assignment (__requires__ = [...] + [...]).

I'm a little worried that nobody has asked for this feature. Are you wanting for yourself to add type annotations to a __requires__ directive? Do you know of any other users who would use this feature if it existed?

I'm also apprehensive as it starts to violate the zen of "preferably one obvious way to do it".

Overall, I'm inclined to say we should do it, especially if the functional approach can be maintained (such as drafted in c9a7094). I promise to be more forgiving for another PR.

@bswck
Copy link
Contributor Author

bswck commented Jun 23, 2024

I'm also apprehensive as it starts to violate the zen of "preferably one obvious way to do it".

But does it?
Depends on how you describe it.

I do believe that we could say there is no more ways to do it with this feature: the obvious way to specify requirements without passing them via sys.argv to pip-run (which already means there are two ways) is to declare a sequence of strings (or a single string! doesn't it already violate the zen? there are three ways already) gathering the requirements.

Syntax isn't what violates the zen here, I believe. I think it's just about some basic flexibility, but that's an opinion, not a helpful fact.

In other words, what I mean is:

__requires__: list[str] = ["jaraco.functools"]

and

__requires__ = ["jaraco.functools"]

are essentially identical, so why not support both, given the cost is so low?

@bswck
Copy link
Contributor Author

bswck commented Jun 23, 2024

On the other hand, there's lots of valid Python syntax that this construct doesn't support, such as mutation (__requires__.append('another')) or multi-value assignment (__requires__, __index_url__ = ...) or non-literal assignment (__requires__ = [...] + [...]).

Yes. Additionally, we do not support (__requires__ := ...) which does set the value at runtime.
But these things were obvious since the beginning.
If Python had something called "build time", we could be more dynamic and actually collect these variables from the "build time", instead of analyzing the code. But I don't think it makes sense to have build time in an interpreted language like Python... does it?

</offtopic>

I'm a little worried that nobody has asked for this feature. Are you wanting for yourself to add type annotations to a requires directive? Do you know of any other users who would use this feature if it existed?

Well, let me be the first one then. I wanted to declare a requirement list without overspecifying the type, that is __requires__: Final = [...] :)

And I'm pretty sure some people would use it. But I can't speak for them.

@bswck
Copy link
Contributor Author

bswck commented Jun 23, 2024

I promise to be more forgiving for another PR.

Alright, see you there!

@jaraco
Copy link
Owner

jaraco commented Jul 2, 2024

Consider using c9a7094 as a baseline (though not required).

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

No branches or pull requests

2 participants