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

Add Python type annotation transformation options #79

Merged
merged 2 commits into from
Apr 28, 2022

Conversation

jbms
Copy link
Owner

@jbms jbms commented Apr 28, 2022

No description provided.

@jbms jbms requested a review from 2bndy5 April 28, 2022 18:53
@jbms
Copy link
Owner Author

jbms commented Apr 28, 2022

I'm not sure what makes sense as far as defaults for all of these options.

@jbms jbms force-pushed the feat-python-type-annotation-transform branch from 7efd151 to 91160f9 Compare April 28, 2022 19:38
Copy link
Collaborator

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

Now that we're using sphinx-jinja, we could probably throw in some docs about domain-specific parameter object types from

DEFAULT_OBJECT_DESCRIPTION_OPTIONS: List[Tuple[str, dict]] = [

docs/conf.py Show resolved Hide resolved
docs/python.rst Show resolved Hide resolved
@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 28, 2022

I'm not sure what makes sense as far as defaults for all of these options.

The booleans make sense. In a perfect world, I think that python_type_aliases would use the actual imported symbol, but using strings makes sense implementation wise.

I can't think of any objections to any of this. Let me give it a spin on my CirPy_nRF24 project...

Copy link
Collaborator

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

I forgot that I have avoided using typing in my CirPy_nRF24 lib because CircuitPython doesn't have the a ported variant of CPython's typing module (due to limited memory resources). The good news is that nothing is broken.

@jbms
Copy link
Owner Author

jbms commented Apr 28, 2022

What do you think about the concise_literals change? Since that is not valid Python syntax, should we still apply that transformation by default?

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 28, 2022

I actually like that change. I think there was a recent PEP that now prefers the | syntax. Primary reason that I didn't take to type hinting in python is because I tend to abuse the dynamic typing of python and the rendered type hints in docs tend to be "wordy" (which makes me think it would only add confusion to the reader's experience).

I guess some square brackets would help differentiate from other data in the signature (like default values). Since python uses the or instead of |, we're not really at risk of ambitiousness between concise literals vs default values

@jbms
Copy link
Owner Author

jbms commented Apr 28, 2022

An example where concise literals are ambiguous is the following:

class Foo:
  pass

def foo(t: Literal[Foo]):
  pass

foo(Foo)

With the concise literals transform, this would be documented as:

foo(t: Foo)

which is incorrect.

@mhostetter
Copy link
Contributor

Is there a case when a Literal with only one option is used? Wouldn't the argument be superfluous at that point? I suspect this case wouldn't come up much in practice.

Really like these additions, by the way!! 🔥

@jbms
Copy link
Owner Author

jbms commented Apr 28, 2022

Probably the 1-argument Literal would only be used as part of a larger type, e.g. Union[int, Literal[False]] or something like that.

There is still an ambiguity:

class Foo: pass
class Bar: pass

def foo(t: Literal[Foo, Bar]): pass

Here foo would be incorrectly documented as:

foo(t: Foo | Bar)

@jbms jbms force-pushed the feat-python-type-annotation-transform branch from 91160f9 to 34cb7cf Compare April 28, 2022 22:01
@jbms
Copy link
Owner Author

jbms commented Apr 28, 2022

I added an additional restriction to only use the concise syntax for constants. That avoids the ambiguity.

@jbms jbms merged commit da6fe28 into main Apr 28, 2022
@jbms jbms deleted the feat-python-type-annotation-transform branch April 28, 2022 22:04
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

Successfully merging this pull request may close these issues.

3 participants