-
Notifications
You must be signed in to change notification settings - Fork 53
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
Refactor to create attrs extension instead of having the code inside our astbuilder #577
Conversation
Codecov Report
@@ Coverage Diff @@
## master #577 +/- ##
==========================================
+ Coverage 91.16% 91.26% +0.10%
==========================================
Files 46 47 +1
Lines 7840 7868 +28
Branches 1701 1709 +8
==========================================
+ Hits 7147 7181 +34
+ Misses 433 427 -6
Partials 260 260
Continue to review full report at Codecov.
|
This PR is also ready for review, it includes only some refactors to isolate the code that handles the attrs 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.
Sorry. I will not have time to review this.
I think that is best to merge this PR.
It is great to see more code using the extension mechansim.
It can be used as an example or documentation of what the extension can do.
At the same time, it helps validates that the extension public API is useful.
Thanks!
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 seems like a good refactoring. Like Adi I'm not sure I can give this a thorough review; it seems like a lot of similar code is moving around, so it's hard to see if there was much in the way of changes. But overall: using extension interfaces for extensions is a good thing :)
This PR refactors the code that handles
attrs
declarations into an extension module.