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

Add support for single version in forge style version "range" #98

Merged
merged 6 commits into from
Jan 14, 2024
Merged

Add support for single version in forge style version "range" #98

merged 6 commits into from
Jan 14, 2024

Conversation

LukenSkyne
Copy link
Contributor

As mentioned in a previous discussion and the resulting issue #95, a version range with only a single version is currently not supported. The open end ranges (e.g.: [1.19,) to include 1.19 and above) seem to work properly, but the closed off variant (e.g.: [1.19.3]) format does not.

This PR adds test cases for these situations as well as a suggested fix which satisfies the new and existing tests ran by npm run test.

@Kir-Antipov
Copy link
Owner

Thanks a lot for your assistance! Also, I greatly appreciate the fact that you included the appropriate tests in the PR too :)

Just a quick note: currently, your implementation treats [1.2.3,] the same way it does [1.2.3]. Although the former is not a valid notation, I would prefer it to be treated as a typo for [1.2.3,), especially considering it was like that before this pull request. Additionally, [,1.2.3] is still treated as (,1.2.3], so it would be a logical thing to do - to mirror that behavior.

Perhaps you could introduce a new separator capturing group for the comma and modify the condition to !to && !separator, or something similar?

P.S. - There's no need to create tests for this particular behavior since it's faulty and shouldn't be really relied upon by users.

@LukenSkyne
Copy link
Contributor Author

Thanks for your feedback!

I'm glad you mentioned the behavior of the mirrored cases, since I haven't considered them in the implementation.
I have added just one test to verify the correct treatment of the "typo" case and introduced the separator group which makes it very easy to check for the single version case.

Btw, thanks for introducing me to named capturing groups, I didn't know they existed before visiting this repo!

@Kir-Antipov
Copy link
Owner

Thank you for your patience. I apologize for the two-month delay in merging this - I've been quite busy with my other commitments.

I have added just one test to verify the correct treatment of the "typo" case and introduced the separator group which makes it very easy to check for the single version case.

Appreciated!

Btw, thanks for introducing me to named capturing groups, I didn't know they existed before visiting this repo!

That's the beauty of open source and contributing to different projects in general - there's always something new to learn :)

I like named capture groups because they provide at least some insight into what's happening in the regular expression when you revisit it months later. And match.groups.separator is definitely more informative than match[2] in code. However, they are either not supported by some other languages or require a different syntax (e.g., Rust, Python, and Go use (?P<name>) syntax), so watch out for that.


Once again, thank you for your time and effort. I hope that my... ahem... brief absence won't discourage you from contributing to the project in the future! :)

@Kir-Antipov Kir-Antipov merged commit cb6e2c9 into Kir-Antipov:dev Jan 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants