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

promisify prompt.get #196

Merged
merged 2 commits into from
Mar 5, 2020
Merged

promisify prompt.get #196

merged 2 commits into from
Mar 5, 2020

Conversation

caub
Copy link
Contributor

@caub caub commented Mar 2, 2020

@gangstead Hi, could you check this very small PR?

It just promisifies prompt.get (if no callback passed)

it'll help for #170

We have to do

const prompt = require('prompt');

const getPrompt = require('util').promisify(prompt.get).bind(prompt);

every time we use prompt, so that PR would be cool

@caub caub force-pushed the promisify branch 2 times, most recently from 8e1ac94 to 34658ba Compare March 2, 2020 20:42
@gangstead
Copy link
Member

I'm not really active on this project any more, so I'm a little hesitant to merge the PR when the build isn't passing.

@caub
Copy link
Contributor Author

caub commented Mar 3, 2020

Ah, I initially put an arrow callback

/home/travis/build/flatiron/prompt/lib/prompt.js:197
  return new Promise((resolve, reject) => {
                                       ^^
SyntaxError: Unexpected token =>

then I changed it to a function for node 0.12 34658ba

The CI should pass, I just don't manage to restart the CI build

It'd be really great if you could merge it, I'm sure many other users would benefit from this promise-style prompt.get

Thanks in advance for your time

@caub
Copy link
Contributor Author

caub commented Mar 3, 2020

Ah, I forgot one arrow fn (there were 2, I changed only 1), thanks CI

It should pass now, sorry

@caub
Copy link
Contributor Author

caub commented Mar 3, 2020

The error now https://travis-ci.org/flatiron/prompt/jobs/657864911#L322 has nothing to do with this PR

@caub
Copy link
Contributor Author

caub commented Mar 4, 2020

@gangstead

  • added a test for prompt.get() as a Promise
  • updated README
  • fixed npm test on recent node versions with an ugly hack (added util.print back because it was removed in recent node versions, I didn't know it existed though:)

@caub caub force-pushed the promisify branch 2 times, most recently from a94cf28 to 52fa1b6 Compare March 4, 2020 23:45
@caub caub mentioned this pull request Mar 4, 2020
@caub caub force-pushed the promisify branch 3 times, most recently from c72a1bd to 0f554ce Compare March 5, 2020 00:04
@gangstead gangstead merged commit 8d5495c into flatiron:master Mar 5, 2020
@caub
Copy link
Contributor Author

caub commented Mar 5, 2020

Thanks @gangstead , do you have npm push access by any chance?

If not, https://www.npmjs.com/package/prompt shows @indexzero and @jcrugzz as collaborators, if they can do a npm publish at some point, that would be cool!

Thanks guys

@gangstead
Copy link
Member

gangstead commented Mar 5, 2020 via email

@caub
Copy link
Contributor Author

caub commented Mar 7, 2020

@indexzero Hey, could you please, when you've a minute, do a npm version minor, git push and a npm publish?

We added a helpful promisified prompt.get in this PR

@apolopena
Copy link

Yeah this would really be nice.

@caub
Copy link
Contributor Author

caub commented Nov 7, 2020

Can any one of the npm collaborators @jcrugzz @indutny @indexzero do something please? we need to sync this new updates on npm

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