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

Remove extraneous combinators #330

Merged
merged 3 commits into from
Feb 5, 2017
Merged

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Feb 4, 2017

Implications for #216

  • Add a note about map being B to S.map documentation.
  • Remove S.S in favor of S.ap and add a note to its documentation.
  • Add a note about join being W to S.join documentation.

Implications for the future

Maybe we should rename the remaining combinators to their full names.

@Avaq Avaq changed the title Av extraneous combinators Remove extraneous combinators Feb 4, 2017
index.js Outdated
//.
//. See also [`C`](#C).
//. This is the `C` combinator from combinatory logic
Copy link
Member

Choose a reason for hiding this comment

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

Three minor points:

  • In this context C should not be rendered as code. The description of I, for example, simply states:

    The I combinator.

  • The sentence should end with a full stop (.).

  • We don't mention combinatory logic in the descriptions of the other combinators, but it seems reasonable to do so here given that flip is not listed in the Combinator section.

I suggest:

This is the C combinator from combinatory logic.

//. > Z.map(S.flip(Math.pow)(2), [1, 2, 3, 4, 5])
//. [1, 4, 9, 16, 25]
//. > S.flip(S.concat, 'foo', 'bar')
//. 'barfoo'
Copy link
Member

Choose a reason for hiding this comment

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

👍

index.js Outdated
@@ -806,13 +776,16 @@
//. In general terms, `compose` performs right-to-left composition of two
//. unary functions.
//.
//. See also [`B`](#B) and [`pipe`](#pipe).
//. This is the `B` combinator from combinatory logic.
Copy link
Member

Choose a reason for hiding this comment

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

See comments about the C combinator above.

@Avaq Avaq force-pushed the av-extraneous-combinators branch from 59075cb to a82f202 Compare February 4, 2017 17:11
//. Takes a binary function and two values, and returns the result of
//. applying the function to the values in reverse order.
//. Takes a curried binary function and two values, and returns the
//. result of applying the function to the values in reverse order.
Copy link
Member

Choose a reason for hiding this comment

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

Let's append this sentence to the paragraph:

Equivalent to Haskell's flip function.

We do this for several other functions, and it seems helpful for those coming from Haskell.

Copy link
Member Author

Choose a reason for hiding this comment

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

We've only done that for functions where the name in Sanctuary differs from the name in Haskell. Are you sure we want to do it for flip, which maps to .. flip?

Copy link
Member

Choose a reason for hiding this comment

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

Haha, fair point. It may not add value. The type signature already indicates that the function takes three arguments (unlike Ramda's version which takes just one).

I leave it to you to decide whether or not to include the sentence. I'm happy either way. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's leave it this way. Once we go down this road we may feel the urge to document how every function relates to Haskell. I'll save us that trouble. ;)

index.js Outdated
@@ -806,13 +776,16 @@
//. In general terms, `compose` performs right-to-left composition of two
//. unary functions.
Copy link
Member

Choose a reason for hiding this comment

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

We could simplify this description. The first paragraph is overly wordy. What do you think of the following?

Composes two unary functions, from right to left. Equivalent to Haskell's (.) function.

That says it all, really.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not really related to this PR, but I'm happy to make that change. ;)

Copy link
Member

Choose a reason for hiding this comment

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

It's not really related, that's true, but an issue entitled "improve all function descriptions" is too daunting to consider. We can at least make small improvements as they occur to us. :)

index.js Outdated
//.
//. See also [`B`](#B) and [`pipe`](#pipe).
//. This is the B combinator from combinatory logic.
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the link to pipe:

See also pipe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh dear. Good catch. ⚡

@Avaq Avaq force-pushed the av-extraneous-combinators branch from 67f215d to eebb9c2 Compare February 5, 2017 09:24
@davidchambers davidchambers merged commit 7acb882 into master Feb 5, 2017
@davidchambers davidchambers deleted the av-extraneous-combinators branch February 5, 2017 13:16
@davidchambers
Copy link
Member

I pushed three commits to #216 to complete the tasks on the to-do list. How do they look, @Avaq?

@davidchambers
Copy link
Member

Maybe we should rename the remaining combinators to their full names.

Once #216 is merged we'll be left with I, K, A, and T. I'm open to renaming these, but I don't think this should delay the v0.12.0 release. :)

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