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 for string literal references #86

Closed
Kigstn opened this issue Jun 10, 2022 · 6 comments
Closed

Support for string literal references #86

Kigstn opened this issue Jun 10, 2022 · 6 comments
Labels
feature New feature or request

Comments

@Kigstn
Copy link

Kigstn commented Jun 10, 2022

Is your feature request related to a problem? Please describe.
Currently, string literal typing (x: "MyClass" instead of x: MyClass) does not generate reference links in the docs, as can be seen here:
image

I have not found any information about this in your planned features, apologies if this is already planned / known.

Describe the solution you'd like
It would be very nice if griffe would parse those and generate reference links.

Describe alternatives you've considered
One could stop using string literal typing, but that is not always feasible because of circular imports, etc.

@Kigstn Kigstn added the feature New feature or request label Jun 10, 2022
@pawamoy
Copy link
Member

pawamoy commented Jun 12, 2022

Hello, thanks for the feature request 🙂

Of course circular imports must be avoided, I completely understand the need for stringified annotations. One thing you could try though, is to use from __future__ import annotations at the top of your modules making use of string annotations. With this import, you'll then be able to simply remove the quotes, as every annotation becomes a string. I cannot guarantee this is the best recommendation for this situation, see some resources on the subject here:

If from __future__ import annotations is not a good fit for you, then we could consider supporting explicit string annotations in Griffe. However, just thinking about it, it seems hard to implement. Indeed, how do you distinguish a regular, literal string from a string that is in fact a reference to an object? Maybe the parser should be made aware of valid cases, like:

  • -> Literal["some_string"]: just a string
  • -> "SomeType": certainly a reference to an object
  • ?

Maybe we could only support explicit uses of ForwardRef?

Happy to get your thoughts on this ☺️

@Kigstn
Copy link
Author

Kigstn commented Jun 18, 2022

Thanks for the recommendation, I will check that out to see how / if that works!

My current main problem is avoiding circular imports, which I sadly cannot fully control because I am working of an external API spec that imports classes from anywhere and I do not want to put all my classes in one giant 40k line file :D

Concerning griffe support:

typing has typing.get_type_hints() which does correctly parse forward references, but only if they are imported in the current context.
image

This fails if classes had to be imported with if typing.TYPE_CHECKING to avoid circular imports:

if typing.TYPE_CHECKING:
    from tests.class_b import B

class A:
    x: int
    y: typing.Literal["some_string"]
    z: "B"

print(A.__annotations__)
print(typing.get_type_hints(A))

--> NameError: name 'B' is not defined

If this is solvable, it may be a decent solution.


The problem with circular imports however does not get solved by this which is arguably as important.

I would imagine the following idea may work but I have not had an opportunity to test it yet.

Take the earlier defined class A for this example. Instead of parsing the args all at once, we'd look at the args one by one now. Let's assume we are done with parsing x and y and go through the process of looking at z in-depth, since the others are straightforward.

  1. Interprete the type with typing -> typing returns NameError since B is not imported
  2. If the interpretation fails with a NameError check there are if typing.TYPE_CHECKING: imports that match the string type -> B was imported with from tests.class_b import B
  3. Link the string type to a reference to the found import without actually importing it (otherwise one would return to the circular import problem) -> {"z": {"name": "B", "reference_location": "tests.class_b.B"}}

I am not really familiar with how griffe does its type and reference parsing, so this may or may not be feasible at all :D

@pawamoy
Copy link
Member

pawamoy commented Jun 18, 2022

Thanks for your feedback 🙂

The issue with your idea is that... we don't use typing.get_type_hints() at all 😄 ! And the reason is... get_type_hints needs the code to be executed! By default Griffe does not execute the code (contrary to its predecessor, pytkdocs), it simply gets the AST (using ast.parse) and visits the tree of nodes. Resolving B is not the issue (Griffe already knows how to do that). The issue is that "B" will be an ast.Const node instead of an ast.Name one, and it could be hard to guess that it should be handled as a forward reference instead of a literal string.

Well, I tried to list "all the different situations" and all I can think about is:

  • Literal["A"]: a literal string
  • "A" anywhere else: a forward reference (tuple["A"], Union["A", Type["A"]], etc.)

...so it might not be that hard!

Can you see any other example where a string without Literal is not a forward reference?

@tristanlatr
Copy link

We use this simple transformer to process annotations in pydoctor: https://github.com/twisted/pydoctor/blob/19e29766d4ad5b8226fe7cb4103776ba508b9c66/pydoctor/astbuilder.py#L1015

@pawamoy
Copy link
Member

pawamoy commented Jun 26, 2022

Thank you @tristanlatr 🙂
I'll see if I can keep an inside_literal state variable in my unparsing functions. That would allow us to avoid transforming the tree.

pawamoy added a commit that referenced this issue Jun 27, 2022
@pawamoy pawamoy closed this as completed Jun 27, 2022
@Kigstn
Copy link
Author

Kigstn commented Jun 29, 2022

Thanks for supporting this. Just installed the latest version and it works like a charm! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants