FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: promisify the "run" command by NoamGaash · Pull Request #269 · extrabacon/python-shell · GitHub

  1. extrabacon
  2. python-shell

Repository navigation

feat: promisify the "run" command - #269

Merged
Almenon merged 4 commits into
extrabacon:masterfrom
NoamGaash:feat/promisify
Feb 11, 2023
ConversationCommits (4)ChecksFiles changed
Merged

feat: promisify the "run" command#269
Almenon merged 4 commits into
extrabacon:masterfrom
NoamGaash:feat/promisify

Conversation

NoamGaash commented May 31, 2022

Copy link
Copy Markdown
Contributor

that's my humble attempt to solve issue #77
thanks for making and maintaining this repo

NoamGaash commented Jun 4, 2022

Copy link
Copy Markdown
Contributor Author

@Almenon

Almenon requested changes Jun 11, 2022
View reviewed changes

Almenon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for submitting the PR! It's a good start. If you want to I can make the below changes, or you can take a shot at them:

  • Check your returns, some of them aren't needed.
  • Having a callback in addition to a promise might be confusing to the person using the API. I'm assuming you left it in as to not cause breaking changes, which is good practice, but the return value is already changed, which means a major semver update. As long as we're making changes lets get rid of the callback param.
  • for consistency it would be good to promisify the runstring method as well.

NoamGaash commented Oct 13, 2022

Copy link
Copy Markdown
Contributor Author

I believe I have found a way to support both the legacy callback-based API and the newer promise-based one.
I believe my implementation has two main drawbacks:

  1. The promise-based implementation cant support providing both stdout and stderr. a promise cannot be both resolved and rejected.
  2. I didn't find a solution for marking specific argument as deprecated, therefore I abuse the console.warning interface

NoamGaash requested a review from Almenon December 29, 2022 15:03

Almenon commented Jan 16, 2023

Copy link
Copy Markdown
Collaborator

Sorry for the delay, I do intend on getting to this eventually.

Almenon commented Jan 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

The problem with trying to support both approaches is that the typing doesn't work out. If you use .then typescript complains because the return value might not be a promise. There might be some fancy typescript syntax for solving this, but I'm just going to simplify and remove the callback option for the next major release. I'm presuming that's fine?

https://i.imgur.com/3dV1D5p.png

Almenon commented Feb 11, 2023

Copy link
Copy Markdown
Collaborator

I'm going to merge this in, publish as v4 so you have your changes, and then make some changes I want to make w/ a v5 release.

Almenon merged commit 4c9e327 into extrabacon:master Feb 11, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Almenon Awaiting requested review from Almenon

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL