-
Notifications
You must be signed in to change notification settings - Fork 11
making sure each of the build-identifier is uint64_t
#5
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
Conversation
| const std::string version_pattern = "^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)" | ||
| "(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))" | ||
| "?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$"; | ||
| const std::string loose_version_pattern = "^v?(0|[1-9]\\d*)(?:\\.(0|[1-9]\\d*))?(?:\\.(0|[1-9]\\d*))" | ||
| "?(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))" | ||
| "?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
author notes:
those 2 patterns weren't modified. just to ease reading it was fragmented into multiple lines
build-identifier is uint64_t
include/semver/semver.hpp
Outdated
| namespace semver | ||
| { | ||
|
|
||
| using build_identifier_part = uint64_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it's used for the type of each numeric part of a version and not just for the pre-release/build identifier's numeric part, could we name it numeric_part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@katzoded Thanks for the contribution! I have some comments regarding the naming, but after fixing that, I'll approve.
@z4kn4fein ,
You are welcome, I addressed the required comments.
include/semver/semver.hpp
Outdated
| semver_exception(const std::string& message) : std::runtime_error(message) { } | ||
| }; | ||
|
|
||
| inline build_identifier_part parse_build_identifier_part(const std::string& version_part) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the previous comment, it might be better to name this method parse_numeric_part().
|
@katzoded Thanks for the contribution! I have some comments regarding the naming, but after fixing that, I'll approve. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
=======================================
Coverage 99.01% 99.02%
=======================================
Files 5 5
Lines 509 512 +3
=======================================
+ Hits 504 507 +3
Misses 5 5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
the reason for this change was to make sure each part of the
build-identifiersis anunsigned long longtheoretically also
unsigned longis 64bit, however it depends on the compilation flagsper only if
LP64is defined in compilation theunsigned longwould be 64bit