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

[fixes #27] Remove bluebird #28

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[fixes #27] Remove bluebird #28

wants to merge 5 commits into from

Conversation

munro
Copy link
Member

@munro munro commented Apr 24, 2019

  • remove bluebird
  • use babel to transpile to support node v4
  • update documentation to show async/await style, and make sure it looks succinct & idiomatic!
  • write tests for async/await, but also keep the old tests to show that it still works with v4
  • make sure tests pass

@farhadabar
Copy link

Hi;
We need to catch child process error events (e.g. ENOENT) before it crashes its parent process.

The following statement does the job.

ps.on('error', function(err) {
console.log('pythonBridge: ' + err);
});

"test:ts": "tsc --lib ES2015 test_typescript.ts && tap test_typescript.js"
"test:js": "tap test-compiled.js",
"test:ts": "tsc --lib ES2015 test_typescript.ts && tap test_typescript.js",
"compile": "babel index.js --out-file index-compiled.js && babel test.js --out-file test-compiled.js"

Choose a reason for hiding this comment

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

I'd suggest adding a prepare script that runs npm run compile.

Here's a good article explaining how it works.

Choose a reason for hiding this comment

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

This removes the need to store a compiled version in git.

@OmgImAlexis
Copy link

OmgImAlexis commented Apr 27, 2020

@munro any chance on this being finished soon?

@munro
Copy link
Member Author

munro commented Apr 27, 2020

Hi;
We need to catch child process error events (e.g. ENOENT) before it crashes its parent process.

The following statement does the job.

ps.on('error', function(err) {

console.log('pythonBridge: ' + err);
});

@farhadabar I think you're probably right about catching errors on ps.on('error', ...), but it's also crazy that that has never come up in 5 years. The API has been really stable so I'd hate to increase the surface area of the API if its unneeded in practice.

If you can create a PR with integration tests showing the need for catching those errors (which I think is very possible), I'd be more than happy to collaborate and come up with an API for dealing with those ps error events. My feeling is that we can trigger those errors by:

  • Passing a bad Python interpreter path
  • Killing Python interpreter during execution
    • using sys.exit
    • using kill -SIGNAL
    • simulating a seg fault
    • simulating out of memory error
  • We could try executing bad Python code, but I think that will get caught

Also, for this PR to remove bluebird, I would love help if you could donate anytime. :)

@munro
Copy link
Member Author

munro commented Apr 27, 2020

@munro any chance on this being finished soon?

@OmgImAlexis If you can donate any time to help with this, that would be greatly appreciated! Just switching the code to not store the compiled code in git may be enough to help get the ball rolling on removing bluebird. Really at the end of this, nobody should notice any difference, except perhaps a slightly lighter npm install.

Also, this was opened awhile ago, so if you see any ES6 idiomatic changes you'd like to make to the code, go for it! My main thing is trying to get LOC as low as possible, as well as dependencies to 0 would be nice—while maintaining the same API.

@chrisblossom
Copy link

I would like to see this done too. Is there anything I can do to help move this forward?

@OmgImAlexis
Copy link

I’ll get on this today. Shouldn’t take me long.

I’ll open a new PR with the updated changes. I’ll just remove bluebird and the compiled code.

@munro
Copy link
Member Author

munro commented Feb 5, 2021

#27 (comment)

just got some insight on what I believe is the best way to handle this—CHANGE COURSE!!!! 🚢 🧭

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.

4 participants