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

Minor improvements to convertors #902

Closed
wants to merge 5 commits into from
Closed

Conversation

JayH5
Copy link
Member

@JayH5 JayH5 commented Apr 18, 2020

  • typing: Make convertors generic over their return types.
  • PathConvertor can inherit from StringConvertor
  • Use r-strings for regex pattern literals.

@JayH5 JayH5 changed the title Minor improvements to converters Minor improvements to convertors Apr 18, 2020
@JayH5 JayH5 mentioned this pull request Apr 18, 2020
raise NotImplementedError() # pragma: no cover

def to_string(self, value: typing.Any) -> str:
def to_string(self, value: Any) -> str:
Copy link
Member Author

Choose a reason for hiding this comment

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

It's not really clear to me what the type of value is. Possibly it can also be T, but the semantics of to_string are not documented so I'm not sure.

@tomchristie
Copy link
Member

Neat bit of work - thanks.

It's a bit of a mixed bag if the Generic typing is an improvement or not, since the implementation style of the way that convertors are used (create them, given the path pattern, use them to populate request.path_params) means that the typing information provided in (say) class FloatConvertor(Convertor[float]): doesn't actually ever become exposed anyplace.

Net result... we might just want to stick with simplicity and continue to under-specify the return type, as we currently do.

Not sure. 🤷‍♂️

@JayH5
Copy link
Member Author

JayH5 commented Apr 21, 2020

Thanks for the thoughtful review. I think I agree that this doesn't add terribly much except checking that the convertors return the expected type. Seems like we're all still figuring out when typing is most useful in Python.

I've reverted the change that made PathConvertor inherit from StringConvertor. I'll leave this for now but feel free to merge or close. Otherwise in a few days I'll close this. (The tests are failing now for some seemingly unrelated reason, though 😞)

@JayH5 JayH5 closed this May 18, 2020
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.

None yet

2 participants