Skip to content

Conversation

@ctaggart
Copy link
Contributor

@ctaggart ctaggart commented Nov 27, 2017

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

@dt-bot
Copy link
Member

dt-bot commented Nov 27, 2017

types/node/index.d.ts

to authors (@DefinitelyTyped/DefinitelyTyped @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @OliverJAsh @eps1lon Microsoft TypeScript (account can't be detected)). Could you review this PR?
👍 or 👎?

@ctaggart
Copy link
Contributor Author

I don't understand the failed tslint on travis-ci. I get this locally:

C:\Users\camer\ts\DefinitelyTyped [http2-constants ≡]> npm run lint node

> definitely-typed@0.0.1 lint C:\Users\camer\ts\DefinitelyTyped
> dtslint types "node"

@typescript-bot
Copy link
Contributor

@ctaggart Please fix the failures indicated in the Travis CI log.

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Nov 27, 2017
@eps1lon
Copy link
Collaborator

eps1lon commented Nov 27, 2017

I checked out your PR and run the linter with a fresh npm install and npm install typescrpt and got the same errors as travis got.

The error however has nothing to do with the PR. The behavior was added with microsoft/dtslint#89. There are currently however 65 packages which use tslint:disable.

@kjin
Copy link
Contributor

kjin commented Nov 27, 2017

Given that the errors have nothing to do with the PR, this looks good to me 👍

@ctaggart
Copy link
Contributor Author

ctaggart commented Dec 1, 2017

@eps1lon, microsoft/dtslint#89 is fixed now. What is the easiest way to get this passing the tests now?

@eps1lon
Copy link
Collaborator

eps1lon commented Dec 1, 2017

#21768 should fix this. So I think you should/can wait for that one to be merged.

After that I'm not so sure. Maybe it's enough to rebase your pr with master to trigger a rerun or if you have to reopen it for travis to recognize the change.

@ctaggart
Copy link
Contributor Author

ctaggart commented Dec 5, 2017

Triggering a new CI build.

@ctaggart ctaggart closed this Dec 5, 2017
@ctaggart ctaggart reopened this Dec 5, 2017
@ctaggart
Copy link
Contributor Author

ctaggart commented Dec 5, 2017

The build passes now that the tslint:disable stuff was sorted out and merged to master.

@typescript-bot
Copy link
Contributor

Approved by a listed owner. PR ready to merge pending express review by a maintainer.

@bowdenk7 bowdenk7 merged commit 8f912d2 into DefinitelyTyped:master Dec 6, 2017
@ctaggart ctaggart deleted the http2-constants branch December 6, 2017 20:04
KSXGitHub pushed a commit to KSXGitHub/DefinitelyTyped that referenced this pull request May 12, 2018
node export http2 constants in standard way
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Popular package This PR affects a popular package (as counted by NPM download counts).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants