michaelsembwever commented on PR #2336: URL: https://github.com/apache/cassandra/pull/2336#issuecomment-1566093290
@jakubzytka, as David says, patches require a minimal approach in the project, along with meeting the style. I know I led you down the garden path by encouraging improvements to UpgradeTestBase to make it easier to understand. apologies for that. The `.contains(..)` bug is now fixed, it is in the `. edgeTouchesTarget(..)` method. And the existing .`contains(..)` method is renamed to `. rangeCoversTarget(..)`. With that in place, you can see that I've only then needed to change the parameter names to the method: `upgradesToCurrentFrom(..)`, `upgradesTo(..)`, `upgradesFrom(..)` and `upgrades(..)`. This should help demonstrate my earlier points that - no existing apidoc or code comments were wrong, and - upgrade and upgradeVersion needs to be lists. To go into more detail… > First of all, the edgeTouchesTarget(Semver from, Semver to, Semver target) has special handling for patch etc. versions. The supported upgrade paths do not go to the granularity of patch versions. > Furthermore, this function may be buggy. Consider a test which defines its applicability from version X.Y.Z This is a valid point, but is not a problem today and out-of-scope. AFAIK no one is passing patch-number explicit versions to these methods. I agree that an assertion that we don't (yet) support patch-specific bounds would be appropriate. -- I've completed the patch for the ticket with an additional commit on my branch. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
