lilydjwg / nvchecker

New version checker for software releases
MIT License
428 stars 69 forks source link

Allow using both `prefix` and `from_pattern` (or document that they’re not compatible/fail if both are defined) #249

Closed Freso closed 8 months ago

Freso commented 8 months ago

I’ve been pulling my hair out for the last 30 minutes trying to figure out what was wrong with my very simple substitution regex (it’s not even a regex at all) trying to get this to work on strings like “Version 0.26 Beta 5” and “Version 0.25”:

prefix = "Version "
from_pattern = " Beta "
to_pattern = "b"

I had to go looking at the code to find https://github.com/lilydjwg/nvchecker/blob/c944cbcac390fb4baafa09bde71c633146abba55/nvchecker/core.py#L285-L288 and see that prefix and from_pattern are mutually exclusive, seemingly based on @yan12125’s opinion that it isn’t reasonable to use both in a package.

Clearly I disagree. :) Yes, I could write a regular expression to extract the "0.26" and "Beta" and "5" and manipulate them accordingly… but it’s much simpler (and more readable/understandable) to see that “Oh, the prefix "Version " gets removed and then " Beta " gets replaced with "b"”. As PEP 20 says: Simple is better than complex. :)

Edit: I’m not actually sure I can write a regex that does what I want/need using Python’s re syntax. What I have right now is '(?i).*(\d(?:\d|\.)*\d)(?: (b)eta (\d+))' and '\1\2\3', but \2 gives a capital B and \2\3 will give an re.error when there’s no "Beta" version. I was also playing with '.*(\d(?:\d|\.)*\d)(?: Beta (\d+))' and '\1b\2', but that will still give the re.error on non‐beta releases, and even if it didn’t, there would be a dangling "b" after the version. Maybe there’s something I’m missing, but I’m not seeing anything in re’s syntax for lowercasing or conditionally using backreferences. I’m starting to think my initial approach of stripping "Version " and replacing " Beta " (if it exists) with "b" would, in fact, be the way to go.

If it is really not desired to allow both, then this should at the very least be documented (as @lilydjwg also noted), and it should probably give an error (or at the very least a notice) that incompatible settings are being used so users know that prefix and from_pattern won’t both be considered (similar to the check for to_pattern).

Freso commented 8 months ago

Oh, I just (now) noticed that it is in the documentation, just at the end of the global options, several paragraphs removed from the descriptions of prefix, from_pattern, and to_pattern. If the current behaviour is strongly desired to be kept, this documentation could probably do to either be moved to the beginning (ie., after The following options apply to every check sources. You can use them in any item in your configuration file.) or into the description of prefix or from_pattern, to_pattern.

lilydjwg commented 8 months ago

It's supported now but note it's still not very powerful. You might need to do post-process outside nvchecker for more complex situations.

yan12125 commented 8 months ago

Thanks for the example that shows allowing both is beneficial. Indeed such cases were not in my brain.

Also, thanks lilydjwg for the update!