Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uprefactor tests, update dependencies and Project API #401
Conversation
codecov-io
commented
Nov 18, 2016
•
Current coverage is 90.02% (diff: 93.84%)
|
|
Thanks for the great work! The only question I have is what did you brea? I don't see what changes require a major version bump. |
| @@ -2,22 +2,18 @@ sudo: false | |||
| language: node_js | |||
| node_js: | |||
| - '6' | |||
| - '5' | |||
| - '4' | |||
This comment has been minimized.
This comment has been minimized.
clayreimann
Nov 20, 2016
Member
How much longer is node4 supported? Maybe we don't drop support for node4 just yet.
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 21, 2016
Author
Contributor
we are not dropping the support for node4, just node 0.12 and earlier. I removed it from the test cause there were some issues with running the test 3 times in parallel. But maybe we should test on node4 only
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
This may be solved by running them serially: https://docs.travis-ci.com/user/customizing-the-build#Limiting-Concurrent-Builds
| ## GitHub Tools | ||
|
|
||
| The team behind Github.js has created a whole organization, called [GitHub Tools](https://github.com/github-tools), | ||
| dedicated to GitHub and its API. In the near future this repository could be moved under the GitHub Tools organization |
This comment has been minimized.
This comment has been minimized.
| * @param {Requestable.callback} [cb] - will receive the pull request information | ||
| * @return {Promise} - the promise for the http request | ||
| */ | ||
| updatePullRequst(number, options, cb) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
@clayreimann You were asking about a breaking change that would necessitate a major version bump. This would be it.
| @@ -152,7 +155,13 @@ class Requestable { | |||
| */ | |||
| _request(method, path, data, cb, raw) { | |||
| const url = this.__getURL(path); | |||
| const headers = this.__getRequestHeaders(raw); | |||
|
|
|||
| const AcceptHeader = (data || {}).AcceptHeader; | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 21, 2016
•
Author
Contributor
the header is call Accept so I wasn't sure. I have no strong feeling about it
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
I think @clayreimann is getting at the usage of PascalCasing instead of camelCasing.
This comment has been minimized.
This comment has been minimized.
| "name": "github-api", | ||
| "version": "2.4.0", | ||
| "name": "@mathieudutour/github-api", | ||
| "version": "3.2.0", |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 21, 2016
Author
Contributor
it's not, I will roll back those changes. I wasn't sure you were interested in merging such a PR so I waited for that first ;)
| (found, member) => member.login === testUser.USERNAME || found, | ||
| false | ||
| ); | ||
| const hasTestUser = members.some((member) => member.login === testUser.USERNAME); |
This comment has been minimized.
This comment has been minimized.
| # switch back to master, build and publish | ||
| git checkout master | ||
| npm run build | ||
| npm publish |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| cache: | ||
| directories: | ||
| - node_modules | ||
| before_install: npm install -g npm@latest |
This comment has been minimized.
This comment has been minimized.
| dedicated to GitHub and its API. In the near future this repository could be moved under the GitHub Tools organization | ||
| as well. In the meantime, we recommend you to take a look at other projects of the organization. | ||
|
|
||
| ## Samples |
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
What's the thought behind removing all this documentation? Are you proposing we drop the link to the generated Docs?
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 21, 2016
Author
Contributor
It's not removed, it's rearranged to fit https://github.com/noffle/art-of-readme
| }); | ||
| ``` | ||
|
|
||
| ```javascript | ||
| import GitHub from 'github-api'; | ||
| var GitHub = require('github-api'); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 21, 2016
Author
Contributor
hum, because for people who are not using babel/ES6, it's an additional cognitive load which can be avoided. For people using ES6, I'm quite sure they know how to use const instead of var.
But again, no strong feeling and happy to roll back if you prefer
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
My opinion is that ECMA2015 is spec now, so there's no need to downgrade.
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 21, 2016
Author
Contributor
sure, but var is not deprecated in anyway, so if we can help people understand the repo, I think it's best, regardless of wether it's matching style guide or not, don't you think? (see #392)
This comment has been minimized.
This comment has been minimized.
clayreimann
Nov 22, 2016
Member
My idea was, and hope is, that we can have at least one example of each of: require vs import and then vs cb to give people the full flavor of the API.
That being said I think that perhaps that means most of our examples are import + then with an example included toward the end of require + cb.
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 22, 2016
Author
Contributor
That might be a solution. Feel free to commit on directly here
| // basic auth | ||
| const gh = new GitHub({ | ||
| var gh = new GitHub({ |
This comment has been minimized.
This comment has been minimized.
| }); | ||
| const me = gh.getUser(); | ||
| var me = gh.getUser(); // no user specified defaults to the user for whom credentials were provided |
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
Another downgrade from ECMA2015, I'll stop mentioning all the ones I see now.
| * @param {Requestable.callback} [cb] - will receive the pull request information | ||
| * @return {Promise} - the promise for the http request | ||
| */ | ||
| updatePullRequst(number, options, cb) { |
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
@clayreimann You were asking about a breaking change that would necessitate a major version bump. This would be it.
| let remoteIssues; | ||
|
|
||
| before(function() { | ||
| before(function(done) { |
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
Mocha allows you to return a promise chain instead of passing the done callback.
| @@ -23,7 +23,7 @@ describe('Project', function() { | |||
| github | |||
| .getUser() | |||
| .createRepo({name: testRepoName}) | |||
| .then(wait()) | |||
| .then(wait(5000)) | |||
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
I generally find arbitrary delays like this in tests to be brittle. Perhaps a better solution is to poll for the repo until it comes back successfully.
| }); | ||
| ``` | ||
|
|
||
| ```javascript | ||
| import GitHub from 'github-api'; | ||
| var GitHub = require('github-api'); |
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
My opinion is that ECMA2015 is spec now, so there's no need to downgrade.
| @@ -152,7 +155,13 @@ class Requestable { | |||
| */ | |||
| _request(method, path, data, cb, raw) { | |||
| const url = this.__getURL(path); | |||
| const headers = this.__getRequestHeaders(raw); | |||
|
|
|||
| const AcceptHeader = (data || {}).AcceptHeader; | |||
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 21, 2016
Member
I think @clayreimann is getting at the usage of PascalCasing instead of camelCasing.
| @@ -349,11 +349,12 @@ describe('Repository', function() { | |||
|
|
|||
| it('should write to repo', function(done) { | |||
| remoteRepo.writeFile('master', fileName, initialText, initialMessage, assertSuccessful(done, function() { | |||
| remoteRepo.getContents('master', fileName, 'raw', assertSuccessful(done, function(err, fileText) { | |||
| wait().then(() => remoteRepo.getContents('master', fileName, 'raw', | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 22, 2016
Author
Contributor
Sometimes the writeFile succeed even though github didn't really create the file yet and the getContents call fails with 404. Wait is there to give time to Github to settle down
This comment has been minimized.
This comment has been minimized.
mtscout6
Nov 22, 2016
Member
Blind wait's a generally very brittle. Can you pull github for the file and if it doesn't show up within 20 seconds then fail?
This comment has been minimized.
This comment has been minimized.
mathieudutour
Nov 22, 2016
Author
Contributor
Indeed, I'll see if I can write an easy to use helper. (This can be done in another PR though)
This comment has been minimized.
This comment has been minimized.
|
For the changelog generation should we have all these commits squashed with the commit message |
|
There's nothing here that I really object to, @mtscout6 look good to you too? |
|
Just the rebase and squash for the change log generation. If that's still a thing for this repo. |
|
How is the changelog generation working exactly? |
|
Changelog generation is manual
…On Nov 29, 2016, 5:45 PM -0600, Mathieu Dutour ***@***.***>, wrote:
How is the changelog generation working exactly?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (#401 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AA4w94TztRRlcNlX7iiIVQ6TgwtIZtIpks5rDLkYgaJpZM4K27nr).
|
|
Very well, I thought it was auto generated from the commit messages. Never mind then this is good to go. |
6ec1d0b
into
github-tools:master
mathieudutour commentedNov 18, 2016
•
edited
michael/githubtogithub-tool/githubupdatePullRequst(user.getEmailsTestRepo(and make them pass)That's a lot of changes. Tell me if you are interested, I'll clean the history and the package.json