Skip to content

Consistent Interface parameter placement #23

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 13, 2021
Merged

Consistent Interface parameter placement #23

merged 1 commit into from
Jan 13, 2021

Conversation

razcore-rad
Copy link
Contributor

With this PR I'm just trying to align the placement of Interface in
the API, it's just cosmetic, but I find that having Interface either
at the beginning (this PR's option) or at the end is desirable.

@garyb
Copy link
Member

garyb commented Jan 12, 2021

I would suggesting putting Interface at the end consistently is the better option - it's the way we usually do it when binding to an effectful JS API.

The reason for that is it plays better with bind and traverse, since you can do things like foo =<< interface... I think for this API that probably isn't going to get much use, but I'd still suggest following it because it's the more common convention. All the web libraries use that convention - I'm not sure about the other node ones that have this kind of situation, but if they differ I'd say they should be changed similarly.

@garyb
Copy link
Member

garyb commented Jan 12, 2021

Hah, I see @thomashoneyman said otherwise in #21 (comment), but personally I still think it's right to put Interface at the end for the reasons I mentioned above.

@thomashoneyman
Copy link
Contributor

thomashoneyman commented Jan 12, 2021

@garyb I'm happy to defer :) I sometimes put these sorts of values as the first argument because it's easier to scan over them if they're in a consistent position (vs. depending on the number of arguments) and because you can partially-apply if you're going to be using the function several times (I think? It's been a while since I've done this). Given that the web libraries already use your suggested placement I think that's enough reason to stick with that, let alone your other reasons.

@razcore-rad
Copy link
Contributor Author

I put it in the first position cause I was thinking about partial application as well, but I'm happy with either as long as it's consistent.

@thomashoneyman
Copy link
Contributor

@razcore-art let's go with @garyb's suggestion!

@razcore-rad
Copy link
Contributor Author

I've updated the PR, should be good to go.

test/Main.purs Outdated
Comment on lines 15 to 19
setLineHandler (\s -> if s == "quit"
then close interface
else do
log $ "You typed: " <> s
prompt interface) interface
Copy link
Contributor

Choose a reason for hiding this comment

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

This can use a bit less indentation with:

Suggested change
setLineHandler (\s -> if s == "quit"
then close interface
else do
log $ "You typed: " <> s
prompt interface) interface
interface # setLineHandler \s ->
if s == "quit" then
close interface
else do
log $ "You typed: " <> s
prompt interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the whitespace so it isn't like either of us! :P

Copy link
Contributor

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

I'd like a look from @garyb but otherwise 👍

"Fix" API functions consistency by placing `Interface` parameter at the
end of the argument list.
@thomashoneyman thomashoneyman merged commit a9e93bf into purescript-node:master Jan 13, 2021
@razcore-rad razcore-rad deleted the api-consistency branch January 13, 2021 19:48
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