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

Using NewType doesn't play nice with sphinx docs #50

Open
davesque opened this issue Jul 27, 2019 · 2 comments
Open

Using NewType doesn't play nice with sphinx docs #50

davesque opened this issue Jul 27, 2019 · 2 comments

Comments

@davesque
Copy link
Contributor

davesque commented Jul 27, 2019

Has anyone figured out a way around this? I generally don't use NewType because I think it's not necessary in the majority of cases, but I ran across this weird behavior recently when I got curious about how sphinx would represent function signatures with NewType annotations:

Index = NewType('Index', int)

def child_ext_private_key(self, i: Index) -> 'ExtPrivateKey':
    ...

...leads to this weirdness:
Screen Shot 2019-07-26 at 11 04 51 PM
Whereas doing things with a simple type alias:

Index = int

def child_ext_private_key(self, i: Index) -> 'ExtPrivateKey':
    ...

...behaves as expected:
Screen Shot 2019-07-26 at 11 05 00 PM
I would expect sphinx to represent a NewType with the name of the type e.g. Index in this case. But, honestly, even that seems problematic. Someone reading the docs won't know what an Index is supposed to be even though it's just an int. Even if there was documentation on a typing module that's part of a package, it doesn't seem reasonable for people to have to look up what is meant by a NewType annotation whenever they want to understand the API of a method.

@cburgdorf
Copy link
Contributor

I haven't seen that docs bug before (but that may just be because we haven't had such public API case documented anywhere or I haven't been paying attention)

But on the actual case, I personally find an Index NewType not very helpful. What are we trying to prevent? Why are people not supposed to call this API with int? The only real reason I could see here is if this API were only meant to be used by the output of another API.

def child_ext_private_key(i: Index) -> PrivateKey:
    ...

def get_index_generated_out_of_whatever() -> Index:
    ...

In that case, it could make sense to prevent usage of int to force the user to use the other API to retrieve an index or to really make them think about index generation.

Doesn't sound like a strong case.

Imho, a strong case for NewType really is something like this:

def get_speed_in_kmh() -> KmH:
    ...

def get_speed_in_mph() -> MpH:
    ...

def get_duration_denver_to_boulder(speed: MpH) -> Minutes:
   ...

def get_duration_hamburg_to_berlin(speed: KmH) -> Minutes:
   ...

Let's ignore the questionable design choice to use localized units for the calculations based on the cities but what's setting the example apart from the index one is that in this case representing the speed (as well as the duration) as just int is ambiguous in a place where ambiguity can result in fatal bugs. If I feed get_duration_hamburg_to_berlin with an int and think it is meant to calcualte with miles when internally it calculates with kilometer things will go wrong. Similarly, if I take the return value of that API as a plain int I'm not sure if it returns seconds, minutes or hours and I might feed the result into another API thinking it were hours when in reality it's minutes.

@davesque
Copy link
Contributor Author

@cburgdorf Nice, I agree. I've actually never used NewType because, aside from the doc bug, it requires too much verbosity in code that a user wants to type check. It also requires a bunch of dummy function calls which technically throw away a few CPU cycles. The NewType for Index was just hypothetical and in the actually code I wrote I did Index = int.

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