nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

ncu-team: retrieve the whole team #185

Closed joyeecheung closed 6 years ago

joyeecheung commented 6 years ago

Do not cap on the 100 limit.

priyank-p commented 6 years ago

I assume that you plan to check on hasNextItems and if so request the list for last totalCount - 100.

BTW i notices something from last time members can't retrieves the list i think (at-least i can't) so we might need to have a not for this somewhere. Sorry i should have brought this up before.

joyeecheung commented 6 years ago

@cPhost The whole pagination thing has already been implemented and used by other queries, see

https://github.com/nodejs/node-core-utils/blob/a15715ad1434ed12287adf15d00ed35123a654a8/lib/request.js#L62-L85

This PR just uses that by passing the path params and fix the query.

joyeecheung commented 6 years ago

BTW i notices something from last time members can't retrieves the list i think (at-least i can't) so we might need to have a not for this somewhere. Sorry i should have brought this up before.

Do you mean you don't have the permission to get the list? Have you configured the read:org scope in your token?

priyank-p commented 6 years ago

Maybe thats it, it says it looks like you have correct authorization ... but not allow to access data. My token is since the repo started so its only for the email one.

Edit: I was using wrong token so it wasn't work, i do have read:org for the one i was suppose to use :)

joyeecheung commented 6 years ago

@nodejs/automation Anyone wants to review this?

priyank-p commented 6 years ago

@joyeecheung this lgtm to me but was waiting for you to fix the tests.

riyadhalnur commented 6 years ago

The coverage reporter failed and not the tests actually

codecov[bot] commented 6 years ago

Codecov Report

Merging #185 into master will decrease coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
- Coverage   93.74%   93.73%   -0.01%     
==========================================
  Files          17       17              
  Lines         655      654       -1     
==========================================
- Hits          614      613       -1     
  Misses         41       41
Impacted Files Coverage Δ
lib/team_info.js 95.65% <100%> (-0.1%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3bb1dd5...809dd41. Read the comment docs.

joyeecheung commented 6 years ago

@cPhost Thanks for catching that..I am probably too used to red CIs these days :( Fixed tests. PTAL

priyank-p commented 6 years ago

@joyeecheung no problem i though you were on it :), anyways the test lgtm since it just involved some promise fixed.