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

#150 Refactoring. Implemented pluggable providers for type hinting. #187

Merged

Conversation

emacsway
Copy link
Contributor

No description provided.

@mcepl
Copy link
Contributor

mcepl commented Oct 29, 2016

By swift browsing through the patch, I have only one question: I see just one test. Could we get some more tests to cover for this whole world? Or is the test coverage carried over from the previous commits?

@mcepl
Copy link
Contributor

mcepl commented Oct 29, 2016

@aligrudi ??? What do you think?

@emacsway
Copy link
Contributor Author

@mcepl , we test behavior, not implementation. Behavior currently is unchanged. Under refactoring (not re-engineering) we don't have to change the tests.

@emacsway
Copy link
Contributor Author

emacsway commented Oct 29, 2016

@mcepl , In future, there will be added providers for PEP-0484 annotations of python3. Also It will be possible to add custom providers by users (using config or DIP), for example, for Django models, etc.

Each new behavior will be tested.

@emacsway
Copy link
Contributor Author

Or is the test coverage carried over from the previous commits?

@mcepl , yes, all behavior covered by old tests , I just renamed the module, because for now type-hinting can use not only docstrings.

New behavior will be added under #146

@emacsway emacsway force-pushed the 150-type_hinting_refactoring branch 6 times, most recently from 15eac20 to 32f7ddc Compare October 30, 2016 19:33
@soupytwist soupytwist merged commit 46b6341 into python-rope:master Nov 22, 2016
@soupytwist
Copy link
Contributor

As @emacsway says, this behavior is already covered by tests, only the implementation has been moved around. Seeing as nothing new is introduced except making it configurable, I don't see any risk in accepting this. Thanks!

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.

3 participants