Add SemanticVersion.valid? & SemanticVersion.parse?

[devnote-dev]

I wasn't sure if this required additional specs given that the underlying implementation (i.e. the regex) is the same. Closes #15020.

[straight-shoota]

Actually, we should still have some basic specs to ensure the methods work.

[devnote-dev]

@straight-shoota can you elaborate on what specifically? All the methods depend on the regex which is already tested in specs.

[straight-shoota]

Just a few simple calls to .valid? and .parse? with a successful and a failing value for each should be fine.
This is to make sure the methods compile and operate generally. Edge cases are covered by the other implementations, but we still need to validate these particular API methods are working.

[straight-shoota]

Can we get at least one successful test for each method? At the moment, even false/nil would fulfill the specs.

SemanticVersion.valid?("1.2.3").should be_true
SemanticVersion.parse?("1.2.3").should eq SemanticVersion.new(1, 2, 3)

[beta-ziliani]

Hi @devnote-dev , thanks a lot for your contributions! We understand your reaction in this comment; having several rounds of reviews is tiring! Let me take a bit more of your time to understand what are we doing wrong.

From our point of view, the comment suggesting specs was clear enough: "add successful and failing cases". Since only half of that was added in the subsequent commit, straight-shoota naturally asked for the other half. This seemed to not be excepcted, and therefore the reaction.

First of all, and we need to make this more obvious to the contributors: whenever you're done with a PR, if you don't mind someone else taking over, you can leave it to us to do the final polish. And second, in order to improve future reviews, could you elaborate on why the first comment failed to transmit the expected outcome?

Thanks once again for making Crystal better!

[devnote-dev]

@beta-ziliani Thank you for your acknowledgement. I don't mind having several rounds of reviews, but I do find these recent ones a bit unnecessary.

I understand and support having full test coverage, but I also believe there's a point where testing is overdone. The .parse method's implementation depends on parse?, so IMO it only really makes sense to test that it raises the correct exception. We know that it will parse correct versions as long as .parse? does. I have similar feelings for testing .valid?: for as long as .parse? will parse correct versions, .valid? will return true for them. The API is very closely intertwined which is why I reacted as I did, it was nothing personal towards @straight-shoota but rather the reasoning for the review (sorry if it came across that way 🙏).

I won't protest this issue as I've seen it become a blocker for valid contributions in the past, but I would like this issue to be taken into consideration for future contributions.

[ysbaddaden]

@devnote-dev I agree it can feel repetitive or be over-testing, but such assumptions means that specs become implementation dependent. What if #parse is refactored and doesn't depend on #parse? anymore, same for #valid?. Specs should catch any issues but they won't, and they fail at their job to protect against regressions and help with refactors.