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

chore(dep): add stringify-package to project source #65

Conversation

Merfoo
Copy link

@Merfoo Merfoo commented Apr 5, 2023

Resolves #49

I considered replacing the stringify-package package with the @npmcli/package-json package as mentioned in #49 (comment) but decided that it'd be less intrusive to this project's code to just copy over the code and tests over from the stringify-package package instead (https://github.com/npm/stringify-package).

I also thought that adding @npmcli/package-json as a dependency a little overkill for this project's singular use case of updating the package.json file's version field.

@Merfoo Merfoo mentioned this pull request Apr 5, 2023
@TimothyJones
Copy link
Member

Thanks for the PR!

One request - the package you've included is under the ISC license, which we must include if we are distributing a copy. Can you drop that in a comment in the source file, maybe alongside a link to the original?

Otherwise, I like your solution, especially as it's such a simple package - and, as you point out, we don't need all the features of the official npm one.

If ever they change the format and we need to do something more complicated to write to this field, we'd have to use the official one instead. But given that's an unlikely scenario (which would also have other far-reaching changes) paying that cost now seems unnecessary.

@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2023

Codecov Report

Merging #65 (5b44502) into master (bc6f792) will increase coverage by 0.05%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   97.52%   97.57%   +0.05%     
==========================================
  Files          29       31       +2     
  Lines        1253     1280      +27     
==========================================
+ Hits         1222     1249      +27     
  Misses         31       31              
Impacted Files Coverage Δ
lib/stringify-package.js 100.00% <100.00%> (ø)
lib/updaters/types/json.js 93.33% <100.00%> (ø)
test/stringify-package.spec.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Merfoo
Copy link
Author

Merfoo commented Apr 5, 2023

I've added the ISC as a comment to the source code and added a link to the stringify-package license as well as requested.

(FYI I'm impressed by the fast response to the PR 😄)

@TimothyJones TimothyJones merged commit 3a959a7 into absolute-version:master Apr 5, 2023
@TimothyJones
Copy link
Member

Thanks! I try to respond as soon as I see it, especially if it's a small PR.

Releasing 11.2.1 now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deprecated warning in deps
3 participants