jakubzytka commented on PR #2336: URL: https://github.com/apache/cassandra/pull/2336#issuecomment-1550159502
> can you please start with this commit: trunk...thelastpickle:cassandra:mck/18506/trunk > this fixes the .contains(..) bug, Are you sure this commit fixes the `.contains()` bug? First of all, the `edgeTouchesTarget(Semver from, Semver to, Semver target)` has special handling for patch etc. versions. This behaviour is never explained. Furthermore, this function may be buggy. Consider a test which defines its applicability from version `X.Y.Z` to some version `X+1.0.0`. If `CURRENT` is `X.Y.Z-1` then this function returns `true` even though the test author specifically prohibited running it. Possibly, this function builds on some assumption that is never explicitly stated, like: "the only valid lower applicability bounds are non-patch etc. versions" I'd recommend using an assertion to express assumptions under which the function is valid. > should make things a lot clearer to understand, the commit in question adds more complexity to already complex code and doesn't fix existing inconsistencies between javadocs or the code. I don't see it as a step in the right direction, considering that a simpler alternative exists in this PR. > without having to touch method signatures (which touches all the other classes) would you kindly elaborate? My PR doesn't change the signatures of the existing public API. > starting off like this will help the review process progress I don't understand what you mean. As already mentioned, the commit 9e32b906fb2fe822be39005ba40de882407e7848 adds additional code, whereas my PR mostly removes code. I don't see how could these two approaches meet. Unless, obviously, my change is based on some invalid assumptions, or doesn't take into account additional assumptions. I admit I struggled with that as I was unable to find documentation of these assumptions. Is there any particular concern why reviewing this PR would be in any way difficult? If so, that's perhaps a reason to improve it. I tried to simplify the code as much as possible. Specifically, I'm removing complex code and replacing it with a simpler alternative. -- 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]
