Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fund: support multiple funding sources #731

Closed
wants to merge 1 commit into from

Conversation

ljharb
Copy link
Contributor

@ljharb ljharb commented Jan 27, 2020

What / Why

Adds support for multiple funding sources in package.json.

References

See npm/rfcs#68

@ljharb ljharb requested a review from a team as a code owner January 27, 2020 23:31
@ljharb ljharb force-pushed the multiple_funding_sources branch 3 times, most recently from 88433bf to 557a856 Compare January 28, 2020 23:52
@mikemimik mikemimik added Community Enhancement new feature or improvement Release 6.x work is associated with a specific npm 6 release Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature labels Jan 29, 2020
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

It would be nice to add more tests with a variety of single vs multiple funding sources tests:

  • In test/tap/utils.funding.js it would be nice to stress it a bit more with cases such as duplicated urls within the same array, also varying between shorthand string vs object (with and without type), etc
  • In test/tap/fund.js it would be nice to add a fixture using multiple funding items.

lib/utils/funding.js Outdated Show resolved Hide resolved
test/tap/utils.funding.js Outdated Show resolved Hide resolved
@darcyclarke darcyclarke self-requested a review February 18, 2020 17:45
@darcyclarke darcyclarke removed the Release 7.x work is associated with a specific npm 7 release label Feb 18, 2020
@darcyclarke darcyclarke added this to the OSS - Sprint 4 milestone Feb 18, 2020
@ljharb
Copy link
Contributor Author

ljharb commented Feb 19, 2020

@ruyadorno updated! i also added tests for the "open URL" multiple sources case, and fixed a bug.

(There's a possible issue with command arg parsing - npm fund . --browser causes the args to be ['.'] and opts.browser to be 'true', but npm fund --browser . causes args to be [] and opts.browser to be . - iow, it doesn't seem to be able to detect that an option is a boolean arg, when it's followed by other args. That seems very out of scope of this PR to fix, though)

Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

just one very small doc request but that can even be added by the release manager 👍 LGTM

lib/fund.js Show resolved Hide resolved
lib/fund.js Show resolved Hide resolved
@ruyadorno
Copy link
Contributor

That seems very out of scope of this PR to fix, though

agreed 👍 + the team has been very busy reworking the entire handling of configs, so the implementation might change a lot in npm@7 - not to say it will fix itself but just that it's not the best moment to play with that 😬

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Enhancement new feature or improvement Release 6.x work is associated with a specific npm 6 release semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants