lilydjwg/nvchecker

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

Freso opened this issue · 3 comments

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

nvchecker/nvchecker/core.py

Lines 285 to 288 in c944cbc

if prefix:
if version.startswith(prefix):
version = version[len(prefix):]
return version
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).

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.

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.

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

Also, thanks lilydjwg for the update!