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

PR: Add type annotations for rope.contrib.generate.create_generate() #641

Merged
merged 9 commits into from Jan 5, 2023
Merged

PR: Add type annotations for rope.contrib.generate.create_generate() #641

merged 9 commits into from Jan 5, 2023

Conversation

edreamleo
Copy link
Contributor

@edreamleo edreamleo changed the title Improve docstring for create_generate PR: Improve docstring for create_generate Jan 4, 2023
rope/contrib/generate.py Outdated Show resolved Hide resolved
Comment on lines 27 to 34
GenerateVal = Union[
"_Generate",
"GenerateClass",
"GenerateFunction",
"GenerateModule",
"GeneratePackage",
"GenerateVariable",
]
Copy link
Member

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.

Copy link
Member

@lieryan lieryan Jan 4, 2023

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.

rope/contrib/generate.py Outdated Show resolved Hide resolved
rope/contrib/generate.py Outdated Show resolved Hide resolved
@lieryan lieryan changed the title PR: Improve docstring for create_generate PR: Add type annotations for rope.refactor.generate.create_generate() Jan 4, 2023
@lieryan lieryan changed the title PR: Add type annotations for rope.refactor.generate.create_generate() PR: Add type annotations for rope.contrib.generate.create_generate() Jan 4, 2023
@edreamleo
Copy link
Contributor Author

@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.

@lieryan
Copy link
Member

lieryan commented Jan 4, 2023

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 master have logical conflict with the feature branch and you want to test that in CI, you shouldn't be merging the latest changes from master into feature branches, as it creates a bunch of unnecessary merge commits and then even more when master is broken.

@edreamleo
Copy link
Contributor Author

@lieryan Thanks for the heads up.

@edreamleo
Copy link
Contributor Author

@lieryan Shall we call this PR good enough for now?

@edreamleo
Copy link
Contributor Author

@lieryan rev 2fc4bb8 works for me (and mypy).

@lieryan
Copy link
Member

lieryan commented Jan 5, 2023

Thanks, @edreamleo

@lieryan lieryan merged commit d24294f into python-rope:master Jan 5, 2023
@edreamleo
Copy link
Contributor Author

@lieryan You're welcome.

I have started active study of Rope's code. Any comments on This ENB would be appreciated.

@edreamleo edreamleo deleted the ekr-ropemode-docstring branch January 5, 2023 17:40
@lieryan lieryan added this to the 1.7.0 milestone Jan 17, 2023
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