team: implemented api for npm team

npm / npm-registry-client

ISC License

264 stars 108 forks source link

team: implemented api for npm team #111

Closed zkat closed 9 years ago

zkat commented 9 years ago

This implements the npm-registry-client side of npm access and npm test outlined in our internal spec. Specifics are subject to change, but this lays out the main architecture (which I'd still like to work on -- though I don't think we can do better than this without rearchitecting the whole library).

Most notable is that I rewrote lib/access.js in such a way that it won't be backwards-compatible, but the APIs and checks and stuff should be mostly the same. This patch will warrant a semver major bump.

othiym23 commented 9 years ago

This is looking good! The factoring is indeed nice and compact! Can we get a validator for team for the methods that are using it?

Another architectural question: is there anything about editing that specifically needs to be in npm-registry-client? Or would it make more sense to have something like getAll or setMany for bulk commands, rather than tying it to the implementation in the CLI?

zkat commented 9 years ago

I need to double-check the validators that are there for stuff that got removed during dev
team.edit is really just setMany -- npm/npm will do its own drop-to-editor magic. I'll rename this one so it's clearer, though. It's gonna need a special case in the cli anyway.

zkat commented 9 years ago

The TODO on it is because @bcoe and @soldair haven't finalized that specific bulk endpoint, so it's likely to change.

othiym23 commented 9 years ago

Two nits that you can attend to or ignore at your discretion (or is it... at your peril? no, pretty sure it's discretion). Getting the asserts extracted out into a shared function makes for really clean and readable code. I really like how readable this turned out! As far as the underlying functionality goes, I'd get a :+1: from @bcoe over mine, because he has a better idea of what the registry is expecting.

othiym23 commented 9 years ago

(To be clear, barring feedback from @bcoe, this LGTM :sheep:.)

zkat commented 9 years ago

Note: Make sure to merge #113 too before/at the same time as releasing this.

coveralls commented 7 years ago

Coverage decreased (-7.6%) to 83.768% when pulling 42a22f78e66877586b356ab97ebe5a0eca813793 on team-api into efe7e5a7daef9a07bf206bfac0d61861eb212c60 on master.