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

rewrite Generator[..., None, None] to Iterator[...] #4

Closed
carljm opened this issue Oct 27, 2017 · 11 comments
Closed

rewrite Generator[..., None, None] to Iterator[...] #4

carljm opened this issue Oct 27, 2017 · 11 comments

Comments

@carljm
Copy link
Contributor

carljm commented Oct 27, 2017

No description provided.

@carljm carljm changed the title generate Iterator[...] instead of Generator[..., None, None] return type for simple generators rewrite Generator[..., None, None] to Iterator[...] Dec 1, 2017
@gvanrossum
Copy link

Note that these aren't fully equivalent. Generators have a close() method but iterators don't.

@carljm
Copy link
Contributor Author

carljm commented Jan 3, 2018

Hmm, good point. Perhaps that ought to be mentioned at http://mypy.readthedocs.io/en/latest/kinds_of_types.html#generators ?

In practice (at least in our codebase) I find the close() method to be rarely/never used, and Iterator[Foo] is much nicer on the eyes than Generator[Foo, None, None]. So I'd still be inclined to provide this (and probably even enable it by default). But this difference is certainly worth a mention in the docs. (It would be provided as a TypeRewriter, so if someone doesn't want it, it'd just be a small config change.)

@gvanrossum
Copy link

gvanrossum commented Jan 3, 2018 via email

@carljm
Copy link
Contributor Author

carljm commented Jan 3, 2018

Done: python/mypy#4424

@iyanuashiri
Copy link
Contributor

Hey @carljm, I want to work on this issue. I have gone through the source code and saw a make_generator function. Is my job to change the return value from Generator[..] to Iterator[..]?

Thank you as you reply

@carljm
Copy link
Contributor Author

carljm commented Mar 13, 2018

Hi @iyanuashiri! Thanks for your interest. We could implement this feature by returning Iterator[yield_type] from make_generator in the case where send_type and return_type are both None. However, given the above discussion I don't think we should do that, because we want to make it easy for someone to opt-out of the rewriting. So instead we should provide this as a TypeRewriter class with a visit_Generator method that inspects the __args__ of the Generator type and rewrites it if the send and return types are None. And then add this new Rewriter to the default rewriter.

@iyanuashiri
Copy link
Contributor

I think I understand what you want me to do. So I should create a class that inherits from TypeRewriter, with a visit_Generator method that inspects the __args__ of the Generator type?

@carljm
Copy link
Contributor Author

carljm commented Mar 14, 2018

Yes, that’s right!

@iyanuashiri
Copy link
Contributor

Hey @carljm, I am sorry it is taking long to send a pull request. I just got a hang of your code and how to implement my solution. Initially I understood what to do but I didn't know how to do it. I will send a PR as soon as possible. Thank you for the patience.

@kevinjqliu
Copy link

TypeRewriter class with a visit_Generator

@carljm I'm confused by the naming convention. Wouldn't we want to name the new method something like rewrite_Generator because the rewrite method in TypeRewriter class will delegate to getattr(typ, 'name', None).

@carljm
Copy link
Contributor Author

carljm commented Sep 19, 2018

@kevinjqliu Yes, you're right, that was just me mis-remembering the API. I meant rewrite_Generator, not visit_Generator.

@carljm carljm closed this as completed in 6436198 Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants