-
Notifications
You must be signed in to change notification settings - Fork 166
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
PR: Add type annotations for rope.contrib.generate.create_generate() #641
Conversation
rope/contrib/generate.py
Outdated
GenerateVal = Union[ | ||
"_Generate", | ||
"GenerateClass", | ||
"GenerateFunction", | ||
"GenerateModule", | ||
"GeneratePackage", | ||
"GenerateVariable", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is accurate annotation, but I think this would be promising too much to the caller.
create_generate()
is a factory method for creating refactoring operation; so rather than documenting the exact types that it can return, we should just document what it is able to promise: create_generate()
will return an object that matches the Refactoring interface.
rope currently doesn't have a formal interface for Refactoring class, but if it does, it would be an Protocol
or abc
that looks somewhat like this:
class Refactoring(typing.Protocol):
def get_changes(self, *args, **kwargs) -> ChangeSet:
...
callers of create_generate()
should only depend on it returning a class that matches the Refactoring interface, and they shouldn't rely on the concrete classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can annotate create_generate()
as returning a more specific GenerateRefactoring
Protocol, which can further add a promise that get_changes()
doesn't take any parameters and that there is also an additional get_location()
method:
class GenerateRefactoring(typing.Protocol):
def get_changes(self) -> ChangeSet:
...
def get_location(self) -> Tuple[Resource, LineNumber (i.e. int)]:
...
this would be a more risky to promise in the API, since if we later change our mind and want to add a parameter to _Generate.get_changes()
, that would need to be an API breaking change.
@lieryan The python 3.7 checks failed, I think after merging your work. The offending statement is: assert sys.version_info <= (3, 7) I'm sure I didn't add that statement. |
Yeah, sorry for the change that broke master, you aren't supposed to be updating this branch from master yet. Generally, unless there is a merge conflict or you have a good reason to suspect that the new changes in |
@lieryan Thanks for the heads up. |
@lieryan Shall we call this PR good enough for now? |
Thanks, @edreamleo |
Per this comment.