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

Feature: version providers #646

Merged
merged 6 commits into from
Feb 12, 2023

Conversation

noirbizarre
Copy link
Member

@noirbizarre noirbizarre commented Dec 28, 2022

Description

This PR is a proposal because I know it has been stated than there won't be an official PEP621.

This PR doesn't focus on PEP621 but on opening the version source of trust by introducing a version provider.

If has been splitted in multiple commits that can be removed from the PR depending on your acceptance.

The first one is simply introducing the entrypoint commitizen.provider and the commitizen provider which is the current scheme (version taken from commitizen configuration) and backward compatible.

The second one introduce some common TOML-based formats (PEP621, poetry, Cargo) as tomlkit is style preserving. Those implementations does not suffer from the regexp side-effect aka. changing any field matching the criteria (happen as soon as you have another version matching in the file) because they are using the format structured access instead of search and replace.

The third one introduce the JSON-based providers for some formats (npm/package.json, composer), mostly to showcase the possibility (and to echo the FAQ entry for package.json/npm projects).
Given the builtin json is not style preserving (I assume an indentation for each one), I would totally understand their removal.

The fourth one is simply a documentation update to be able to have proper snippets with title in the documentation.

The last one is exactly #647 but was required to make this PR CI green. See the previous build failing on some totally unrelated (and untouched) cases.

This PR is backward compatible (aka. it is possible not to change the version provider as well as using the pep621 provider while handling both Cargo.toml and package.json with version_files), tested and documented.

Note: this PR use the same extension mechanism as #632 and it is totally possible to extract all implementations to only keep the commitizen one given they can be provided as an external package.

Edit: Added an extra commit to add an scm provider for #641. It handles tag_format reverse parsing. I discovered an issue (I will submit) on the tag_format handling: while devrelease is documented as being a variable placeholder in tag_format, it is not handled as a template variable but appended to the tag (.devX when submitted as --devrelease X) whatever it's position is in the template, even if it is not present in the template.

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

@codecov
Copy link

codecov bot commented Dec 28, 2022

Codecov Report

❗ No coverage uploaded for pull request base (v3@a2cad36). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@          Coverage Diff          @@
##             v3     #646   +/-   ##
=====================================
  Coverage      ?   97.98%           
=====================================
  Files         ?       41           
  Lines         ?     1834           
  Branches      ?        0           
=====================================
  Hits          ?     1797           
  Misses        ?       37           
  Partials      ?        0           
Flag Coverage Δ
unittests 97.98% <0.00%> (?)

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

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

@noirbizarre noirbizarre force-pushed the feature/version-provider branch 3 times, most recently from 99a0895 to 16c8449 Compare January 1, 2023 22:55
@noirbizarre noirbizarre force-pushed the feature/version-provider branch 4 times, most recently from 9baa4b0 to fd05fbf Compare January 5, 2023 13:03
@woile
Copy link
Member

woile commented Jan 7, 2023

Wow! Thanks, it's an interesting idea, @Lee-W what do you think? I think it could definitely work.

docs/faq.md Outdated Show resolved Hide resolved
@woile
Copy link
Member

woile commented Jan 7, 2023

Do you think we could do something like this for this #150 ?
It would be interesting if it works as pluggins, and by default we support semver + pep440

@noirbizarre
Copy link
Member Author

Yes, I think this pattern can also be applied to customize the versioning scheme. We can definitively imagine another feature splitting the versioning schemes into a commitizen.scheme entrypoint plugins too (I don't want to tease, I need deeper investigation but if this is OK for you, I can definitively try working on this later)

@noirbizarre noirbizarre force-pushed the feature/version-provider branch 3 times, most recently from 0175ed7 to 9e6af7c Compare January 10, 2023 02:56
@Lee-W
Copy link
Member

Lee-W commented Jan 11, 2023

Wow! Thanks, it's an interesting idea, @Lee-W what do you think? I think it could definitely work.

I'm really interested in this one! But kinda out of bandwidth to take a deeper look 🤦‍♂️ Will do so when I'm not that busy

@davidlday
Copy link
Contributor

I came across this PR while looking for a way to ensure cz doesn't clobber dependency versions in package-lock.json. Using a JSON query solves it, as you'd implemented in the JsonProvider. Would love to see this PR accepted!

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.

Hi @noirbizarre , it's way more interesting that I previously thought! I've browsed through it and found no big issue. Wondering if there's any review order you want us to do between 645 and 646?

@Lee-W
Copy link
Member

Lee-W commented Jan 23, 2023

I've also rebased the latest master back to v3 which leads to a few conflicts 🤦‍♂️

@noirbizarre
Copy link
Member Author

There is no order or dependency between #645 and this one, but I would start with this one because it is ready to review.
Rebased on updated v3 branch, conflict resolved 👍🏼

@Lee-W
Copy link
Member

Lee-W commented Jan 26, 2023

Thanks for the prompt reply! Let me start with this one 👍

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.

@woile I think we're almost ready to merge this one except for a few minor changes. I'm planning on merging it this week. Let me know if you want to take a deeper look

commitizen/providers.py Outdated Show resolved Hide resolved
commitizen/providers.py Outdated Show resolved Hide resolved
tests/commands/test_version_command.py Outdated Show resolved Hide resolved
tests/test_version_providers.py Show resolved Hide resolved
DEFAULT = "commitizen"


class VersionProvider(ABC):
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of using protocols?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this precise case, I prefer ABC over Protocol because:

  • I expect both a constructor and methods (I could move the config in the method signatures, but this makes more verbose code without benefits in my opinion)
  • it is intended for user/community to extend this, using ABC force to implement methods and raise an explicit error if not, which I think is easier to understand (so more discoverable for them, less support for you)

But if you prefer Protocol over ABC I can switch 👍🏼

Copy link
Member

Choose a reason for hiding this comment

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

@woile What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer Protocol 🤣 but using ABC is fine as well. Point 2 is the key, with Protocols people would have to use mypy to catch the problems, I like the explicit error.

@noirbizarre noirbizarre force-pushed the feature/version-provider branch from 973b646 to 78c3dd7 Compare January 27, 2023 00:57
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.

Thanks @noirbizarre for the prompt update! I'm good with both ABC and Protocol. Once that discussion is resolved, I think we're good to merge this one

@noirbizarre noirbizarre force-pushed the feature/version-provider branch from 78c3dd7 to 5672eaf Compare February 11, 2023 15:52
@noirbizarre
Copy link
Member Author

noirbizarre commented Feb 11, 2023

Rebased, conflicts solved. ✅

Can we merge this one please 🙏🏼 ? (because, it's starting to represent a lot of extra work for me to maintain those branches up to date for the past 2 months 😅)

@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 and removed discussion pr-status: wait-for-modification labels Feb 12, 2023
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.

Thanks @noirbizarre for your prompt update!

@woile I'm planning on merging it today. Let me know if you have any thought :)

@woile
Copy link
Member

woile commented Feb 12, 2023

Let's go! thanks for the awesome work!

@Lee-W Lee-W merged commit 5489177 into commitizen-tools:v3 Feb 12, 2023
@noirbizarre noirbizarre deleted the feature/version-provider branch February 13, 2023 09:23
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.

None yet

4 participants