fix(provider): fix npm provider to update package-lock.json and npm-shrinkwrap.json if they exist #813
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #813 +/- ##
==========================================
+ Coverage 97.33% 97.43% +0.09%
==========================================
Files 42 42
Lines 2104 2141 +37
==========================================
+ Hits 2048 2086 +38
+ Misses 56 55 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
commitizen/providers.py
Outdated
| @@ -1,6 +1,7 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| import json | |||
| import jsonpath_ng # type: ignore | |||
There was a problem hiding this comment.
May I know why do we need #type: ignore here
There was a problem hiding this comment.
Also, may I know why do we need to install this package?
There was a problem hiding this comment.
I'd also like to know, is it not possible to update it accessing like a json?
There was a problem hiding this comment.
The problem came from trying to access the correct element in "packages". Here's the sample JSON:
{
"name": "whatever",
"version": "0.1.0",
"lockfileVersion": 2,
"requires": true,
"packages": {
"": {
"name": "whatever",
"version": "0.1.0"
},
"someotherpackage": {
"version": "0.1.0"
}
}
}The json path expression is $.packages.''.version, but I couldn't get that to work via the json object directly.
And the # type: ignore is there b/c the library doesn't appear to have type hints, nor is there anything available for it on typeshed.
There was a problem hiding this comment.
Okay - so not sure why I was struggling with using the json.loads and updated the dict, but I have that working now. Will remove the dependency on jsonpath_ng.
commitizen/providers.py
Outdated
| @@ -166,6 +167,87 @@ class NpmProvider(JsonProvider): | |||
| filename = "package.json" | |||
|
|
|||
|
|
|||
| class Npm2Provider(VersionProvider): | |||
There was a problem hiding this comment.
As mentioned in the issue, I'd suggest we update the NpmProvider directly, but I'm not a JavaScript users. Would love to see how @woile thinks.
There was a problem hiding this comment.
I think so as well, if package-lock.json and or npm-shrinkwrap.json exists they should be updated.
There was a problem hiding this comment.
Makes sense. I'll go back and update the existing NpmProvider and take out the Npm2Provider.
There was a problem hiding this comment.
Should I change the PR from feat to fix then?
There was a problem hiding this comment.
Hi @davidlday , thanks for your contribution! The overall implementation is great! Just a few minor thing needs to be fixed
There was a problem hiding this comment.
@davidlday Awesome! Just left one minor suggestion. After rebasing, we're good to go
tests/test_version_providers.py
Outdated
| @pytest.mark.parametrize( | ||
| "pkg_lock_content,pkg_lock_expected,pkg_shrinkwrap_content,pkg_shrinkwrap_expected", | ||
| ( | ||
| (None, None, None, None), | ||
| (NPM_LOCKFILE_JSON, NPM_LOCKFILE_EXPECTED, None, None), | ||
| (None, None, NPM_LOCKFILE_JSON, NPM_LOCKFILE_EXPECTED), | ||
| ( | ||
| NPM_LOCKFILE_JSON, | ||
| NPM_LOCKFILE_EXPECTED, | ||
| NPM_LOCKFILE_JSON, | ||
| NPM_LOCKFILE_EXPECTED, | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
| @pytest.mark.parametrize( | |
| "pkg_lock_content,pkg_lock_expected,pkg_shrinkwrap_content,pkg_shrinkwrap_expected", | |
| ( | |
| (None, None, None, None), | |
| (NPM_LOCKFILE_JSON, NPM_LOCKFILE_EXPECTED, None, None), | |
| (None, None, NPM_LOCKFILE_JSON, NPM_LOCKFILE_EXPECTED), | |
| ( | |
| NPM_LOCKFILE_JSON, | |
| NPM_LOCKFILE_EXPECTED, | |
| NPM_LOCKFILE_JSON, | |
| NPM_LOCKFILE_EXPECTED, | |
| ), | |
| ), | |
| ) | |
| @pytest.mark.parametrize( | |
| "pkg_shrinkwrap_content, pkg_shrinkwrap_expected", | |
| ( | |
| (NPM_LOCKFILE_JSON, NPM_LOCKFILE_EXPECTED), | |
| (None, None) | |
| ) | |
| ) | |
| @pytest.mark.parametrize( | |
| "pkg_lock_content, pkg_lock_expected", | |
| ( | |
| (NPM_LOCKFILE_JSON, NPM_LOCKFILE_EXPECTED), | |
| (None, None) | |
| ) | |
| ) |
There was a problem hiding this comment.
@Lee-W - Applying suggestion now. Sorry, you made this suggestion earlier, and I wasn't getting it. Makes sense to me now.
|
Hi @davidlday , could you please rebase from the master branch so that we can merge it? You can do it through |
…k.json, and npm-shrinkwrap.json closes commitizen-tools#804
…hrinkwrap.json if they exist closes commitizen-tools#804
…nd npm-shrinkwrap.json if they exist closes commitizen-tools#804
|
@Lee-W - apologies, been awhile since I've done a rebase. Let me know if I messed anything up. |
No worry 🙂 Just want to fix a CI issue along with this PR. |
closes #804
Description
Added new version provider
npm2which usesjsonpath_ngto update version in$.version$.version,$.packages.''.version$.version,$.packages.''.versionpackage-lock.jsonandnpm-shrinkwrap.jsonmay or may not exist.Checklist
./scripts/formatand./scripts/testlocally to ensure this change passes linter check and testExpected behavior
When using
npm2as the version provider, the following files will have the project's version number updated appropriately:package.json(required)package-lock.json(optional)npm-shrinkwrap.json(optional)Steps to Test This Pull Request
Test cases added to
tests/test_version_providers.py.Additional context