Fix luminex tests and clean-up #4063
Conversation
There was a problem hiding this comment.
How long do you want to keep this overload around? Just enough to verify via a TeamCity build completes cleanly and there aren't any other overrides in our codebase? Until we ship 23.3?
I appreciate wanting to help catch any third-party overrides but also don't want to keep this around forever. If we don't delete it immediately, I'd keep the Deprecated tag and maybe even make it throw UnsupportedOperationException to help flush out any other third-party callers.
Assuming code uses the Override annotation (like the Flow and Luminex implementations do) the compilation will fail even without the final method.
| public void deleteProtocol(ExpProtocol protocol, User user) throws ExperimentException | ||
| // Please use new overload with additional String parameter, this version is no longer called, so any custom implementation may get missed. | ||
| // Not sure how else to catch overrides as caller is modified. This should be a breaking change, to ensure any custom Assay Providers are updated. | ||
| final public void deleteProtocol(ExpProtocol protocol, User user) throws ExperimentException |
There was a problem hiding this comment.
Can you not keep the @Deprecated annotation here?
There was a problem hiding this comment.
I could, but I think the better option would be to use Josh's suggestion to just remove it. That will cause any that are using the @Override annotation to fail. The key is we want these Overrides to get updated to the new signature, as any custom clean-up code won't get called because of the inheritance of the new method in AbstractAssayProvider as experienced with the Luminex assay.
There was a problem hiding this comment.
I doubt there are many/any AssayProvider implementations of this method outside of LabKey repos. I vote for yanking the method, adding a release note, and moving on.
Sigmonia
force-pushed
the
fb_fixLuminexDelete
branch
from
7442428 to
bb3e2d6
Compare
Rationale
Test failures due to custom assay protocol clean up https://teamcity.labkey.org/buildConfiguration/LabkeyTrunk_DailySuites_DailyDPostgres/2250071?buildTab=tests&name=luminex&expandedTest=build%3A%28id%3A2250071%29%2Cid%3A719
Related Pull Requests
Changes
deleteProtocolfinal so we can catch any custom implementations since the original calling location uses the new signature.