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

Fix callback invocation for readline.question #38

Merged
merged 3 commits into from
Jun 30, 2024

Conversation

youngnh
Copy link
Contributor

@youngnh youngnh commented Jan 9, 2024

Description of the change

I've made two small changes to what I think are bugs. The first bug prevents the question and question' functions from properly invoking the callback they receive, and the second is a small one-character typo in the options passed to createInterface

  1. To fix the callback not being invoked, I created a EffectFn1 and passed that to the ffi impls. The previous code was passing a function that returned an Effect but never actually ran that effect.

  2. Fixed typo in options passed to createInterface: s/crlDelay/crlfDelay/


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

Copy link
Member

@garyb garyb left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@brubar
Copy link

brubar commented Jun 9, 2024

Just a friendly ping for this patch to be merged.

Without it, the Readline functions question and question' (both
the Effect and the Aff versions) are useless: they just hang forever.

@garyb garyb merged commit 3d94029 into purescript-node:master Jun 30, 2024
1 check passed
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