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

Add version_type option to make version compatibly with semver #686

Merged
merged 8 commits into from
Apr 19, 2023

Conversation

Apkawa
Copy link
Contributor

@Apkawa Apkawa commented Mar 13, 2023

Description

I decided to use it in a rust project but cargo crashed when trying to use the prerelease versions.

I have added an optional setting that solves my problem for now.

version_type

Choose version type

  • pep440 - default version provider.
    • prerelease - 1.0.1a0
    • devrelease - 1.0.1dev0
    • dev and pre - 1.0.1a0.dev0
  • semver - semver compatibly provider. Added "-" delimiter
    • prerelease - 1.0.1-a0
    • devrelease - 1.0.1-dev0
    • dev and pre - 1.0.1-a0-dev0

TODO: renamed pre to long name, like alpha, beta, ..etc via additional option semver_long

[tool.commitizen]
version_type = "semver"

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

Steps to Test This Pull Request

$ cz bump --check-consistency --no-verify --prerelease alpha --version-type=semver --dry-run
bump: version 0.1.0-a2 → 0.1.0-a3
tag to create: v0.1.0-a3

Additional context

#150

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Patch coverage: 94.11% and project coverage change: -0.19 ⚠️

Comparison is base (e34de55) 98.11% compared to head (f644bae) 97.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##               v3     #686      +/-   ##
==========================================
- Coverage   98.11%   97.92%   -0.19%     
==========================================
  Files          41       42       +1     
  Lines        1855     1928      +73     
==========================================
+ Hits         1820     1888      +68     
- Misses         35       40       +5     
Flag Coverage Δ
unittests 97.92% <94.11%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commitizen/version_types.py 90.74% <90.74%> (ø)
commitizen/bump.py 100.00% <100.00%> (ø)
commitizen/changelog.py 100.00% <100.00%> (ø)
commitizen/cli.py 94.11% <100.00%> (ø)
commitizen/commands/bump.py 98.14% <100.00%> (+0.02%) ⬆️
commitizen/commands/changelog.py 98.91% <100.00%> (+0.02%) ⬆️
commitizen/defaults.py 100.00% <100.00%> (ø)

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@woile
Copy link
Member

woile commented Mar 13, 2023

Hey this is amazing! Thanks for taking the time to do it.

We have to think of a different name instead of version_provider, as we are using that on v3, for the file providing the version, see this

@Apkawa
Copy link
Contributor Author

Apkawa commented Mar 13, 2023

Hey this is amazing! Thanks for taking the time to do it.

We have to think of a different name instead of version_provider, as we are using that on v3, for the file providing the version, see this

hmm, maybe version_format?

@woile
Copy link
Member

woile commented Mar 13, 2023

I was thinking of version_type, because version_format is the format used for the tag, e.g: if you want to have a tag v$version

@woile
Copy link
Member

woile commented Mar 13, 2023

Please also add the setting to config.md

@woile
Copy link
Member

woile commented Mar 13, 2023

@Lee-W wanna take a look? I left some minor comments, but in general it looks good. Do you have any thoughts or thing we should we aware of?

I'm thinking, once we merge this, we should rebase v3.

@woile
Copy link
Member

woile commented Mar 13, 2023

You can run ./scripts/format to fix the linting errors.

@Apkawa
Copy link
Contributor Author

Apkawa commented Mar 13, 2023

Oh, i checked only 3.9. Hmm

@Apkawa
Copy link
Contributor Author

Apkawa commented Mar 13, 2023

Error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond. (api.github.com:443)

Please restart checks

@Apkawa
Copy link
Contributor Author

Apkawa commented Mar 13, 2023

Python package / python-check (3.7, windows-latest) (pull_request)
image
hm, blinked test?

@Lee-W
Copy link
Member

Lee-W commented Mar 13, 2023

I'm on a trip these 2 weeks and probably won't be able to review it.

@Apkawa Apkawa changed the title Add version_provider option to make version compatibly with semver Add version_type option to make version compatibly with semver Mar 13, 2023
@AliSajid
Copy link

AliSajid commented Apr 9, 2023

Bumping this since this is really important for me.

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Apkawa This feature is amazing! Thanks for sending this awesome PR. I think we're almost ready to merge this one. I left a few minor comments. Let me know your thought.

@@ -50,6 +50,9 @@ def __init__(self, config: BaseConfig, args):
"tag_format"
)

version_type = self.config.settings.get("version_type")
self.version_type = version_type and version_types.types[version_type]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we capitalize types? i.e. version_types.TYPES[version_type]. Or maybe VERSION_TYPES might be an even better name?
It took me some time to find out version_type and version_type are different



class VersionProtocol(_Protocol):
def __init__(self, _version: typing.Union[Version, str]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I prefer from typing import Union, Optional a bit more

parts.append(".".join(str(x) for x in version.release))

# Pre-release
if version.pre is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I prefer if not version.pre a bit more

docs/config.md Outdated
@@ -21,6 +21,7 @@
| `use_shortcuts` | `bool` | `false` | If enabled, commitizen will show keyboard shortcuts when selecting from a list. Define a `key` for each of your choices to set the key. [See more][shortcuts] |
| `major_version_zero` | `bool` | `false` | When true, breaking changes on a `0.x` will remain as a `0.x` version. On `false`, a breaking change will bump a `0.x` version to `1.0`. [major-version-zero] |
| `prerelease_offset` | `int` | `0` | In special cases it may be necessary that a prerelease cannot start with a 0, e.g. in an embedded project the individual characters are encoded in bytes. This can be done by specifying an offset from which to start counting. [prerelease-offset] |
| `version_type` | `str` | `pep440` | Select a version type from the following options [`pep440`, `semver`]. Useful for non python projects. [See more][version_type] |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: could you please change non python to non-python? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

tag_exists = git.tag_exist("0.2.0-a0")
assert tag_exists is True

with open(tmp_version_file, "r") as f:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: do you think we could try the following?

for version_file in (tmp_version_file, tmp_commitizen_cfg_file):
    with open(version_file, "r") as f:
        assert "0.2.0-a0" in f.read()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you agree with this, maybe we could try this technique in other tests in this PR as well

Comment on lines 890 to 901
# PRERELEASE
tmp_version_file = tmp_commitizen_project.join("__version__.py")
tmp_version_file.write("0.1.0")
tmp_commitizen_cfg_file = tmp_commitizen_project.join("pyproject.toml")
tmp_version_file_string = str(tmp_version_file).replace("\\", "/")
tmp_commitizen_cfg_file.write(
f"{tmp_commitizen_cfg_file.read()}\n"
f'version_files = ["{tmp_version_file_string}"]\n'
f'version_type = "semver"'
)

create_file_and_commit("feat: new user interface")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These setups are similar across different tests. Would it be possible for us to make them pytest fixtures?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I wrote a simple fixture returning a function with parameters ea7ac93#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128R30


out, _ = capsys.readouterr()

assert out == "## 0.3.0-a1 (2022-02-13)\n\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we unify the test way to use pytest-regressions as well?



@pytest.mark.parametrize(
"test_input,expected",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: please add an additional space i.e., test_input, expected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Lee-W Lee-W added pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check wait-for-merge labels Apr 16, 2023
@Apkawa
Copy link
Contributor Author

Apkawa commented Apr 16, 2023

hmm. CI failed. but i didn't touch dependencies. Do I need to merge the master onto a branch?

@Lee-W
Copy link
Member

Lee-W commented Apr 17, 2023

hmm. CI failed. but i didn't touch dependencies. Do I need to merge the master onto a branch?

I just found out the root cause. It seems to relate to https://about.codecov.io/blog/message-regarding-the-pypi-package/. Could you please remove codecov = "^2.0" in pyproject.toml and try again? Thanks!

@Lee-W
Copy link
Member

Lee-W commented Apr 17, 2023

Other than that, everything looks good to me

@woile
Copy link
Member

woile commented Apr 17, 2023

Could you point this to v3? v3 removes codecov dep and the tests are working fine.

I'll be working on v3 this week, and I would like this to be part of it 💪🏻

@Apkawa
Copy link
Contributor Author

Apkawa commented Apr 17, 2023

Ok. I fixed CI.

@woile
Copy link
Member

woile commented Apr 17, 2023

Could you point the PR to the branch v3? thanks

@Apkawa Apkawa changed the base branch from master to v3 April 17, 2023 09:06
@Apkawa
Copy link
Contributor Author

Apkawa commented Apr 17, 2023

Could you point the PR to the branch v3? thanks

Done. Failed not my test.

In #703 test_changelog_incremental_with_merge_prerelease need add @pytest.mark.freeze_time("2023-04-16") fixture.

i may fix it.

@woile
Copy link
Member

woile commented Apr 17, 2023

Thanks, if you can also rebase your pr on top of V3 so we avoid the merge commit. I can fix the tests myself this week

@Apkawa
Copy link
Contributor Author

Apkawa commented Apr 17, 2023

Thanks, if you can also rebase your pr on top of V3 so we avoid the merge commit. I can fix the tests myself this week

Ok, i can do a rebase to one commit, but I'm afraid that if I do a force push, this PR will close.

@woile
Copy link
Member

woile commented Apr 18, 2023

I'm asking for a rebase, not a squash. Otherwise the timeline will be dirty.
You can do something like:

git remote add upstream https://github.com/commitizen-tools/commitizen.git
git rebase upstream/v3

and then follow the instructions, fix the conflict and continue the rebase until it's done git rebase --continue

After that, you can do a force push and the PR will remain open

…ompatible with semver

Signed-off-by: apkawa <apkawa@gmail.com>
… rename to `pep440`

Signed-off-by: apkawa <apkawa@gmail.com>
Signed-off-by: apkawa <apkawa@gmail.com>
…rsion subclass

Signed-off-by: apkawa <apkawa@gmail.com>
Signed-off-by: apkawa <apkawa@gmail.com>
…ersion_type=semver` option

Signed-off-by: apkawa <apkawa@gmail.com>
… issue with freeze_time

Signed-off-by: apkawa <apkawa@gmail.com>
@Apkawa
Copy link
Contributor Author

Apkawa commented Apr 18, 2023

I'm asking for a rebase, not a squash. Otherwise the timeline will be dirty. You can do something like:

git remote add upstream https://github.com/commitizen-tools/commitizen.git
git rebase upstream/v3

and then follow the instructions, fix the conflict and continue the rebase until it's done git rebase --continue

After that, you can do a force push and the PR will remain open

done, please check

@woile woile merged commit c3d2c91 into commitizen-tools:v3 Apr 19, 2023
@woile
Copy link
Member

woile commented Apr 19, 2023

Thank you so much! 🎉

woile pushed a commit that referenced this pull request Apr 23, 2023
* feat(bump): version_provider=semver optional option to make version compatible with semver

Signed-off-by: apkawa <apkawa@gmail.com>

* refactor(bump): version_provider rename to version_type; `pep` option rename to `pep440`

Signed-off-by: apkawa <apkawa@gmail.com>

* docs(bump): add `version_type` info to config.md

Signed-off-by: apkawa <apkawa@gmail.com>

* refactor(bump): to use VersionProtocol interface instead packaging.Version subclass

Signed-off-by: apkawa <apkawa@gmail.com>

* test(bump): `VersionProtocol` workaround mypy for py==3.7

Signed-off-by: apkawa <apkawa@gmail.com>

* fix(changelog): `changelog` command does not find version tag with `version_type=semver` option

Signed-off-by: apkawa <apkawa@gmail.com>

* refactor: minor review fixes

* test(changelog): fix test_changelog_incremental_with_merge_prerelease issue with freeze_time

Signed-off-by: apkawa <apkawa@gmail.com>

---------

Signed-off-by: apkawa <apkawa@gmail.com>
@noirbizarre noirbizarre mentioned this pull request May 1, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants