-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Remove Bit.register and Bit.index #10996
Conversation
One or more of the the following people are requested to review this:
|
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.
As it stands, this PR hasn't done the complete job; the register
and index
arguments from the constructor also need removing, as do most of the actual methods from the class. It'll become almost a completely opaque object. All the private attributes of the object would be deleted as well, since they're no longer necessary.
Pull Request Test Coverage Report for Build 7262608220
💛 - Coveralls |
Based on how complicated cleaning (see 1ucian0#44) I think it is better to merge this before to warn others the API is changing. |
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 needs a release note about the removal, but given we seem to be going the direction of getting rid of the public components and try to sort out the backend behaviour that we're still accidentally relying on after, the code changes seem complete.
reno in 3f16114 |
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.
Thanks!
For linking purposes: the work of trying to disentangle the inner |
Summary
Fixes #10744