refactor tests, update dependencies and Project API #401

mathieudutour · 2016-11-18T21:13:55Z

That's a lot of changes. Tell me if you are interested, I'll clean the history and the package.json

codecov-io · 2016-11-18T21:29:44Z

Current coverage is 90.02% (diff: 93.84%)

No coverage report found for master at 78a98b1.

Powered by Codecov. Last update 78a98b1...f998a39

clayreimann

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.

clayreimann · 2016-11-20T16:21:19Z

.travis.yml

@@ -2,22 +2,18 @@ sudo: false
 language: node_js
 node_js:
  - '6'
-  - '5'
-  - '4'


How much longer is node4 supported? Maybe we don't drop support for node4 just yet.

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 may be solved by running them serially: https://docs.travis-ci.com/user/customizing-the-build#Limiting-Concurrent-Builds

clayreimann · 2016-11-20T18:00:49Z

README.md

+## 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


Was moved to org

clayreimann · 2016-11-20T20:23:43Z

lib/Repository.js

-    * @param {Requestable.callback} [cb] - will receive the pull request information
-    * @return {Promise} - the promise for the http request
-    */
-   updatePullRequst(number, options, cb) {


@clayreimann You were asking about a breaking change that would necessitate a major version bump. This would be it.

clayreimann · 2016-11-20T20:25:54Z

lib/Requestable.js

@@ -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;


acceptHeader?

the header is call Accept so I wasn't sure. I have no strong feeling about it

I think @clayreimann is getting at the usage of PascalCasing instead of camelCasing.

yes I understood

clayreimann · 2016-11-20T20:29:51Z

package.json

-  "name": "github-api",
-  "version": "2.4.0",
+  "name": "@mathieudutour/github-api",
+  "version": "3.2.0",


Is this diff still relevant?

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 ;)

clayreimann · 2016-11-20T20:36:55Z

test/team.spec.js

-               (found, member) => member.login === testUser.USERNAME || found,
-               false
-            );
+            const hasTestUser = members.some((member) => member.login === testUser.USERNAME);


👍 Yay conciseness!

clayreimann · 2016-11-20T20:41:32Z

release.sh

+# switch back to master, build and publish
+git checkout master
+npm run build
+npm publish


Isn't this done by CI?

indeed, I'll roll back also

mtscout6 · 2016-11-21T13:58:06Z

.travis.yml

 cache:
  directories:
    - node_modules
-before_install: npm install -g npm@latest


What's the reasoning for not using the latest version of npm?

mtscout6 · 2016-11-21T14:00:00Z

README.md

-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


What's the thought behind removing all this documentation? Are you proposing we drop the link to the generated Docs?

It's not removed, it's rearranged to fit https://github.com/noffle/art-of-readme

mtscout6 · 2016-11-21T14:00:49Z

README.md

 });
 ```

 ```javascript
-import GitHub from 'github-api';
+var GitHub = require('github-api');


Why downgrade from ECMA2015?

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

My opinion is that ECMA2015 is spec now, so there's no need to downgrade.

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)

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.

That might be a solution. Feel free to commit on directly here

mtscout6 · 2016-11-21T14:01:05Z

README.md


 // basic auth
-const gh = new GitHub({
+var gh = new GitHub({


Again downgrading from ECMA2015?

mtscout6 · 2016-11-21T14:02:03Z

README.md

 });

-const me = gh.getUser();
+var me = gh.getUser(); // no user specified defaults to the user for whom credentials were provided


Another downgrade from ECMA2015, I'll stop mentioning all the ones I see now.

mtscout6 · 2016-11-21T14:05:58Z

lib/Repository.js

-    * @param {Requestable.callback} [cb] - will receive the pull request information
-    * @return {Promise} - the promise for the http request
-    */
-   updatePullRequst(number, options, cb) {


@clayreimann You were asking about a breaking change that would necessitate a major version bump. This would be it.

mtscout6 · 2016-11-21T14:12:23Z

test/issue.spec.js

   let remoteIssues;

-   before(function() {
+   before(function(done) {


Mocha allows you to return a promise chain instead of passing the done callback.

mtscout6 · 2016-11-21T14:54:29Z

test/project.spec.js

@@ -23,7 +23,7 @@ describe('Project', function() {
      github
        .getUser()
        .createRepo({name: testRepoName})
-        .then(wait())
+        .then(wait(5000))


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.

mtscout6 · 2016-11-21T14:56:25Z

README.md

 });
 ```

 ```javascript
-import GitHub from 'github-api';
+var GitHub = require('github-api');


My opinion is that ECMA2015 is spec now, so there's no need to downgrade.

mtscout6 · 2016-11-21T14:58:17Z

lib/Requestable.js

@@ -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;


I think @clayreimann is getting at the usage of PascalCasing instead of camelCasing.

mtscout6 · 2016-11-22T15:28:40Z

test/repository.spec.js

@@ -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',


Why the wait?

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

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?

Indeed, I'll see if I can write an easy to use helper. (This can be done in another PR though)

Agreed, I'm fine with it in another PR as well.

mtscout6 · 2016-11-23T14:47:34Z

For the changelog generation should we have all these commits squashed with the commit message chore: fix tests

clayreimann · 2016-11-28T17:37:20Z

There's nothing here that I really object to, @mtscout6 look good to you too?

mtscout6 · 2016-11-29T20:37:06Z

Just the rebase and squash for the change log generation. If that's still a thing for this repo.

mathieudutour · 2016-11-29T23:45:27Z

How is the changelog generation working exactly?

clayreimann · 2016-11-30T03:10:38Z

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).

mtscout6 · 2016-11-30T14:50:31Z

Very well, I thought it was auto generated from the commit messages. Never mind then this is good to go.

mathieudutour added 29 commits November 18, 2016 12:35

add getContributorStats

a8ae05f

add labels methods

f5e6fa3

add updateStatus and updateRepository method

26fa048

nice readme

7c1684e

add project api and fix tests

a5e3dbb

3.0.0

cff3973

improve release script

4f46d10

3.0.1

83292fd

remove prepublish script

332456b

request all pages for cards

8b8c4fd

3.0.2

0505bb3

add getEmails

68ef1f6

3.1.0

cda2c42

remove polyfill

99c1aeb

3.1.1

0c5546f

drop support for node < 4

be44fd1

do not hardcode project preview in requestable

c12ad57

3.2.0

308aced

test on node6 only

3ec2837

add coverage

21e50ec

fix lint

2a0bdf2

fix travis.yml

ee8af3f

fix tests

5418512

fix scope

71a6326

add tests for project

5dd868f

add some timeout to settle

077736b

fix _request204or404

55db033

remove clearRepo

068476d

fix typo

97ac33f

mathieudutour mentioned this pull request Nov 18, 2016

WIP: Fix linting that is failing the CI #400

Closed

remove unnesserary setTimeout

1605b89

clayreimann reviewed Nov 20, 2016

View reviewed changes

mathieudutour added 7 commits November 21, 2016 11:53

fix conflicts

a80922c

fix callbackErrorOrThrow

121066c

add some delay

9877d9e

add some delay

45a6ef3

add more delay

fe146a5

test on node 4 and 5

c9de004

add more waiting

e1ae825

mtscout6 reviewed Nov 21, 2016

View reviewed changes

mathieudutour added 3 commits November 21, 2016 14:15

fix concurrency issues

fd5d57b

delete created org project

39dc920

fix typo

ac41f60

mtscout6 reviewed Nov 21, 2016

View reviewed changes

mathieudutour added 2 commits November 21, 2016 22:47

wait more

e0bbf7e

fix typo

f998a39

mtscout6 reviewed Nov 22, 2016

View reviewed changes

clayreimann merged commit 6ec1d0b into github-tools:master Nov 30, 2016

refactor tests, update dependencies and Project API #401

refactor tests, update dependencies and Project API #401

Conversation

mathieudutour commented Nov 18, 2016 • edited Loading

codecov-io commented Nov 18, 2016 • edited Loading

Current coverage is 90.02% (diff: 93.84%)

clayreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieudutour Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtscout6 commented Nov 23, 2016

clayreimann commented Nov 28, 2016

mtscout6 commented Nov 29, 2016 • edited Loading

mathieudutour commented Nov 29, 2016

clayreimann commented Nov 30, 2016 via email

mtscout6 commented Nov 30, 2016

mathieudutour commented Nov 18, 2016 •

edited

Loading

codecov-io commented Nov 18, 2016 •

edited

Loading

mathieudutour Nov 21, 2016 •

edited

Loading

mtscout6 commented Nov 29, 2016 •

edited

Loading