-
Notifications
You must be signed in to change notification settings - Fork 255
Fix/Refactor: Prerelease #435
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
Fix/Refactor: Prerelease #435
Conversation
@@ -46,7 +54,7 @@ def from_variable(config_str: str): | |||
""" | |||
path, variable = config_str.split(":", 1) | |||
pattern = ( | |||
rf'{variable} *[:=] *["\']{PatternVersionDeclaration.version_regex}["\']' | |||
rf'{variable} *[:=] *["\']{version_regex}["\']' |
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.
replaced to use a single source of truth
@@ -57,7 +65,7 @@ def from_pattern(config_str: str): | |||
regular expression matching the version number. | |||
""" | |||
path, pattern = config_str.split(":", 1) | |||
pattern = pattern.format(version=PatternVersionDeclaration.version_regex) | |||
pattern = pattern.format(version=version_regex) |
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.
replaced to use a single source of truth
continue | ||
matches = re.match(r"v?(\d+.\d+.\d+)", commit_message) | ||
if matches: | ||
match = re.search(rf"{release_version_pattern}", commit_message) |
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.
replaced to use a single source of truth
|
||
match = re.search(rf"{pattern}", i.name) |
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.
replaced to use a single source of truth
@@ -14,5 +14,8 @@ force_grid_wrap=0 | |||
use_parentheses=True | |||
line_length=88 | |||
|
|||
[flake8] | |||
max-line-length = 120 |
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.
should I remove this again?
if config.get("version_source") == "tag": | ||
return get_current_release_version_by_tag() | ||
else: | ||
return get_current_release_version_by_commits() |
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.
get_current_release_version
should give us the last not-prerelease version.
As the config file does not give us any history, we have to analyse commits here instead of just looking into the config file.
release_version_pattern = f"(\d+.\d+.\d+(?!{prerelease_pattern}))" | ||
|
||
release_version_regex = rf"{release_version_pattern}" | ||
version_regex = rf"{version_pattern}" |
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.
I added this variables as single source of truth for regex version matching, for ease of use once as string and once as regex.
6611e10
to
6ef2404
Compare
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.
The changes so far look good to me, although there are some conflicts which need to be fixed.
Are you still working on this or is it ready to be merged?
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.
👍
6ef2404
to
73059ed
Compare
rebased |
@danth / @williamluke4 |
@jacksbox Done! EDIT: since merging it seems that the tests are now failing. |
@danth probably we should rollback for now - until we decided / come up with a more secure way of parsing the commit history. sorry for introducing this issues :( |
Just picked up this change in my pipeline. Seems the negative lookback in the release version check is stripping all but the first digit of the patch version. debug: Parsing current version: path=PosixPath('src/analytics_calc/init.py') pattern='version *[:=] *"\'["\']' num_matches=1 Tested the current regex in isolation against the current version and I am indeed seeing a stripped version number returned as match[1] - (\d+.\d+.\d+(?!-beta.\d+)) matched on 0.0.24-beta.0 gives 0.0.2 Removing the "-" from before beta in the regex seems to provide the desired output 🤷 |
Ready for review
Related to #434
When digging into the issue I encountered some more misalignments in my previous provided code for the prerelease feature.
With this PR I want to fix this issues and also do some normalisation work in the package.
@danth @williamluke4 @relekang
Curiously, locally all tests pass. I checked and the test fail because of methods which traverse the git log...Locally all commits starting from the current branch are analysed (ending with the init commit) - but in the ci pipeline, there is only one commit (Some merge commit I can't find nowhere else... ). Any Idea why this is happening and how to prevent it?
Solved: The problem was, that by default githubs
actions/checkout@v2
checks out a single merge commit. It has to be instructed to checkout the actual brach with its full history.Main changes:
Unsolved Issues:
Possible improvements:
There are now two similar methods to get the current version:
get_current_version
: gets the last version, whatever is the last tag or in the config fileget_current_release_version
: gets the last not-prerelease version (this is needed to determine the correct level_bump)for ease of use (they are mostly used together) and further development, this two methods could be merged and return a tuple
(current_version, current_release_version)