Skip to content

Remove length parameter to setPrompt #21

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

Merged
merged 1 commit into from
Jan 12, 2021
Merged

Remove length parameter to setPrompt #21

merged 1 commit into from
Jan 12, 2021

Conversation

razcore-rad
Copy link
Contributor

length is nowhere in the node-readline documentation. Seems something left over from long ago.

This has been bugging me for a while and since this came up to my attention recently I decided to update setPrompt and remove the length parameter which isn't used by node.

@thomashoneyman
Copy link
Contributor

Wow -- the only version I can even find which uses this parameter is Node v0.10, which is certainly beyond our maintenance window. As of the last version we need to maintain (Node v10) this parameter doesn't exist, so I think this is good to go.

@thomashoneyman
Copy link
Contributor

@razcore-art could you also bump the PureScript version in CI to v0.14.0-rc5 to fix the CI failure?

@razcore-rad
Copy link
Contributor Author

I've bumped the CI to the specified version.

I also moved the Interface parameter to the function to be in the same (1st) position so it's predictable. If this isn't wanted let me know and I'll revert.

@thomashoneyman
Copy link
Contributor

I think it may be sensible to move the Interface to always be the first argument to these functions, but I'd rather have that in a separate PR so that others can weigh in on the change. It's not something I'd like to jump into too quickly and I'm not sure how worthwhile a change it is. In almost all functions Interface is the only argument, so it could be considered first or last; it's only in question, setPrompt, and setLineHandler that we have a divergence.

For the meantime could you revert that change, and open a separate pull request? That way I can merge at least this PR which removes the length parameter.

@razcore-rad
Copy link
Contributor Author

I updated the PR to include only the CI version upgrade

@thomashoneyman thomashoneyman merged commit feacadf into purescript-node:master Jan 12, 2021
@razcore-rad razcore-rad deleted the update-setprompt branch January 12, 2021 09:59
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.

2 participants