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

Lift required override on deprecated method in MetricLoader #609

Merged

Conversation

QubitPi
Copy link
Contributor

@QubitPi QubitPi commented Dec 30, 2017

Address issue #608

Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

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

Needs a changelog

@QubitPi QubitPi changed the title Lift required override on deprecated method in MetricLoade Lift required override on deprecated method in MetricLoader Jan 17, 2018
@QubitPi QubitPi force-pushed the lift-required-override-on-deprecated-method-in-metric-loader branch from 1fade64 to bb6e6f5 Compare January 17, 2018 21:49
@QubitPi QubitPi merged commit 67946a7 into master Jan 17, 2018
@QubitPi QubitPi deleted the lift-required-override-on-deprecated-method-in-metric-loader branch January 17, 2018 22:05
@archolewa
Copy link
Contributor

I'm a little late to the party, but I think it's a really bad idea to "deprecate" a method and then have it throw an unsupported operation exception. If a method now makes such little sense that we can't support it, then we should just delete it. All else being equal, compile errors >>>>>>> runtime errors.

Especially since this is a trivially easy thing to fix for people using the old method: add an extra parameter to the method signature. The implementation doesn't need to be touched at all.

@archolewa
Copy link
Contributor

archolewa commented Jan 17, 2018

Especially since the other implementation is a default as well!

Which means that if someone adds a new implementation, their class will compile, and then explode at runtime by default.

@archolewa
Copy link
Contributor

Frankly, I think we should just make the "new" method no longer default and delete the old method if we don't want to make people provide an implementation of the deprecated method.

@QubitPi
Copy link
Contributor Author

QubitPi commented Jan 17, 2018

According to doc, deprecated method can only be removed after a new official release.

I'm OK if you open a new PR or roll back this.

@archolewa
Copy link
Contributor

archolewa commented Jan 17, 2018

Eh, I don't think I care enough to go reverting code. This is fairly minor, and our codebase isn't widely used. That just feels petulant to me.

I do think we should clean this up for the next minor rev though, which we're probably past due for (current version: 0.9.118).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants