search

Kir-Antipov / mc-publish

🚀 Your one-stop GitHub Action for seamless Minecraft project publication across various platforms.
MIT License
216 stars 19 forks source link

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

Closed LukenSkyne closed 6 months ago

LukenSkyne commented 8 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 6 months ago

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! :)