Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update definition to include the .reflect method, replaces .inspect method #10

Merged
merged 3 commits into from
Apr 26, 2016

Conversation

jacobdr
Copy link
Contributor

@jacobdr jacobdr commented Apr 22, 2016

The .inspect method was removed by this commit, and had already been superseded by .reflect.

Bluebird API reference

Closes issue #9

@jacobdr
Copy link
Contributor Author

jacobdr commented Apr 22, 2016

Notes on potential merge strategies:

  1. Only bump the version in the typings registry to v3.3.5 to point at the merged commit. Bluebird@3.3.5 is the latest release with no other API changes

  2. Backport this change to the v3.3.4 and 3.2.1 typings registry definition since .inspect was deprecated well before those releases

@louy
Copy link
Member

louy commented Apr 26, 2016

LGTM 👍

@louy
Copy link
Member

louy commented Apr 26, 2016

I'd go with the second merge strategy. i.e. don't keep the buggy version in the registry.
Any thoughts on how we should keep both version 2 and version 3 definitions in the same repo? Maybe something like what we have in env-node?

@jacobdr
Copy link
Contributor Author

jacobdr commented Apr 26, 2016

I have never maintained a typings library, so I want to be clear about your suggestion. Is your suggestion to have the registry file look something like:

{
  "versions": {
      "2.X": "github:typed-typings/npm-bluebird/2.X/ENTRYPOINT.D.TS#17e3625f149665a0c20b818f3a772f56e6d746b7",
      "3.X": "github:typed-typings/npm-bluebird/3.X/ENTRYPOINT.D.TS#asdf25f149665a0c20b818f3a772f56e6d746b7",
    }       
  }

With 2.X and 3.X being root directories of this repository?

@louy
Copy link
Member

louy commented Apr 26, 2016

Kind of. Here's a real life example.

{
  "versions": {
    "3.0.0": "github:typed-typings/npm-bluebird/3#COMMIT",
    "2.0.0": "github:typed-typings/npm-bluebird/2#COMMIT"
  }
}

@louy
Copy link
Member

louy commented Apr 26, 2016

Do you want to remove .settle before I merge this PR btw?

@jacobdr
Copy link
Contributor Author

jacobdr commented Apr 26, 2016

Yes... I'll update the PR now.

On Tue, Apr 26, 2016 at 9:51 AM, Louy Alakkad notifications@github.com
wrote:

Do you want to remove .settle before I merge this commit btw?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#10 (comment)

@louy
Copy link
Member

louy commented Apr 26, 2016

Awesome. Thank you.

@louy louy merged commit 92126fa into typed-typings:master Apr 26, 2016
@lhecker
Copy link
Contributor

lhecker commented May 25, 2016

@jacobdr @louy You guys got the API wrong. 😕
.reflect() doesn't return PromiseInspection but a Bluebird<PromiseInspection>. See here.
(The comment about it being a synchronous inspection is thus wrong.)

Should I send PRs or do you want to handle it?

@jacobdr
Copy link
Contributor Author

jacobdr commented May 25, 2016

Looks like you are right -- it does return a Promise to a PromiseInspection. In regard to the comment, it is a holdover from the original definitions, which were written for the Bluebird 2.x inspect method but not removed.

If you want, feel free to go ahead and submit a PR -- otherwise I am working on another PR to fix some other differences from the Bluebird 2 -> 3 upgrade, and can roll it in there.

@lhecker
Copy link
Contributor

lhecker commented May 25, 2016

If you don't mind it'd be cool if you could add those changes into your PR, since it's a very simple issue anyways. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants