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

Handle missing patch version by making it optional in regex matching. #417

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

rajkumarGosavi
Copy link
Contributor

Given that the server version is not a proper major.minor.patch format,
example: 16.0

then the version parsing fails.

To avoid this, Added a fallback version regex, so that if the actual regex fails then we check with the fallback version regex,
further since the server version does not contain the patch number setting the patch number to 0 by default.

Something I found while going through the pulseaudio repo, as per my understanding they might be creating major.minor style tags for dev or testing.
https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/master/git-version-gen?ref_type=heads

Closes #339

@AXDOOMER
Copy link
Collaborator

AXDOOMER commented Oct 4, 2023

I'm sure it could be done using a single regex. If the last match group is empty, then use 0 instead for "patch" rather than doing another check using another regex which is essentially the same. No need for two regexes.

@rajkumarGosavi
Copy link
Contributor Author

rajkumarGosavi commented Oct 5, 2023

@AXDOOMER
Here we matching a pattern \.(\d+) before matching .*?, to handle it using a single regex pattern

we can do something like https://go.dev/play/p/pIFHrZlNwPN

diff:
.*?(\d+)\.(\d+)\.(\d+).*?
.*?(\d+)\.(\d+)\.?(\d+)?.*?

Making the last . and digits optional by using ?.

Let me know If this works.

@AXDOOMER
Copy link
Collaborator

AXDOOMER commented Oct 6, 2023

Yes, I think that's how you'd do it.

The result of versionRegex.FindStringSubmatch will always be 4 and if the matchgroup for the minor or patch number are empty, they should be set to 0. If the match for the major number is empty, then there's an error because this is totally unexpected.

@rajkumarGosavi rajkumarGosavi force-pushed the version-regex-fix branch 3 times, most recently from aef4e24 to 00b7e15 Compare October 6, 2023 12:46
@AXDOOMER
Copy link
Collaborator

AXDOOMER commented Oct 6, 2023

Thanks @rajkumarGosavi

Please move the res[3] == "" bloc just above major, err = strconv.Atoi(res[1]) so that Atoi conversions are still all grouped together, then I think the fix is complete.

@rajkumarGosavi rajkumarGosavi force-pushed the version-regex-fix branch 2 times, most recently from 80ae074 to e09f676 Compare October 7, 2023 03:40
@rajkumarGosavi rajkumarGosavi changed the title add a fallback regex to check version Handle missing patch version by making it optional in regex matching. Oct 7, 2023
@rajkumarGosavi
Copy link
Contributor Author

@AXDOOMER I have implemented the changes.

Thank you.

@AXDOOMER AXDOOMER merged commit e5c4d36 into noisetorch:master Oct 10, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"couldn't parse server version, regexp didn't match version"
2 participants