Skip to content

add add_method_to_class() function for plugins.common #8245

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 23, 2020
Merged

add add_method_to_class() function for plugins.common #8245

merged 1 commit into from
Jan 23, 2020

Conversation

mkurnikov
Copy link
Contributor

  • move mypy.plugins.common.add_method() to SemanticAnalyzerPluginInterface and to SemanticAnalyzer
  • change old implementation to call new one, to preserve backwards compatibility
  • replace first ctx parameter with cls: ClassDef

Fixes #8244

@mkurnikov mkurnikov force-pushed the add-method-method branch 2 times, most recently from d3ec436 to a2b63a1 Compare January 6, 2020 08:11
@msullivan msullivan self-requested a review January 7, 2020 00:54
@mkurnikov mkurnikov closed this Jan 7, 2020
@mkurnikov mkurnikov reopened this Jan 7, 2020
msullivan
msullivan reviewed Jan 7, 2020
View reviewed changes
Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

I think I would prefer to just add a new function to mypy.plugins.common instead of moving into the semantic analyzer proper, unless you have a compelling reason to move it

@mkurnikov
Copy link
Contributor Author

The only reason was to make the fact that it could only be used for SemanticAnalyzer phase clearer. Does it need to be another function, or could I change a signature?

@mkurnikov mkurnikov changed the title Move add_method() to the SemanticAnalyzerPluginInterface split ClassDefContext in add_method() signature into cls and api Jan 8, 2020
@msullivan
Copy link
Collaborator

I'd prefer to add a new function, I think, to avoid breaking compatibility unnecessarily. It is kind of unfortunate, though, since it was a mistake really.

@mkurnikov mkurnikov changed the title split ClassDefContext in add_method() signature into cls and api add add_method_to_class() function for plugins.common Jan 22, 2020
@mkurnikov
Copy link
Contributor Author

I've updated, not sure about naming, let me know what you think

@msullivan msullivan merged commit 7570779 into python:master Jan 23, 2020
@msullivan
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msullivan msullivan msullivan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add_method() should be usable in the get_dynamic_class_hook() too
2 participants
@mkurnikov @msullivan