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

Make the invoker documentation a little more understandable to newbies like myself #2116

Merged
merged 5 commits into from
Jan 22, 2022
Merged

Make the invoker documentation a little more understandable to newbies like myself #2116

merged 5 commits into from
Jan 22, 2022

Conversation

krainboltgreene
Copy link
Contributor

No description provided.

src/invoker.js Outdated
*
* var sliceFrom = R.invoker(1, 'slice');
* // A function with one arguments
Copy link
Member

Choose a reason for hiding this comment

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

s/arguments/argument/

src/invoker.js Outdated
* const firstCreditCardSection = invoker(2, "slice", 0, 4)
* firstCreditCardSection("4242 4242 4242 4242") // => Function<...>
*
* // Instead you must immediately invoke the invoker (better wording tbd)
Copy link
Member

Choose a reason for hiding this comment

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

How about:

"Since invoker returns a curried function, you may partially apply it to create the function you need."

@CrossEye
Copy link
Member

CrossEye commented Mar 8, 2017

Although these changes (with the suggestions from @buzzdecafe) are a definite improvement, I wonder if we shouldn't go further and stop lying with "Turns a named method ..." We do not start with a method/function, only with a name. And the same invoker-derived function can easily be called on objects with different types, and with different functions associated with the given name:

const indexOf = invoker(1, 'indexOf');
indexOf('bar', ['foo', 'bar', 'baz']); //=>1 (Array.prototype.indexOf)
indexOf('x', 'Supercalifragilisticexpialidocious'); //=> 21 (String,prototype.indexOf)

I'm still not sure what the right phrasing is, but even with the proposed changes, this is still somewhat misleading.

@krainboltgreene
Copy link
Contributor Author

Changes made @buzzdecafe, but also feel free to make changes.

@buzzdecafe
Copy link
Member

@CrossEye is right, the first sentence of the existing documentation is misleading at best. How about:

"Given a Number arity and a String method name, invoker returns a function that takes arity arguments and a context object, and invokes the method with the arguments in the context specified object."

Is that better? 🤔

@krainboltgreene
Copy link
Contributor Author

Given an arity (Number) and a name (String) the invoker() function returns a curried function that takes arity arguments and a context object. It will "invoke" the name'd function (a method) on the context object.

@buzzdecafe
Copy link
Member

@krainboltgreene i think yours is definitely an improvement on mine

@krainboltgreene
Copy link
Contributor Author

@buzzdecafe Committed!

src/invoker.js Outdated
*
* The returned function is curried and accepts `arity + 1` parameters where
* the final parameter is the target object.
* Given an `arity` (Number) and a `name` (String) the `invoker()` function
Copy link
Member

Choose a reason for hiding this comment

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

i think you can omit the parens after invoker. Otherwise, this looks good to me.

@buzzdecafe
Copy link
Member

There are some linting errors, but otherwise, this LGTM.

🐄

@customcommander
Copy link
Member

Summary of changes:

  1. Rebased against ramda's master branch
  2. Fixed merge conflict
  3. Fixed linter errors
  4. Address @buzzdecafe latest comment
  5. Used const instead of var in example (a pull request predating this one did something similar; so just being consistent here)

@ramda/ramda-docs Good to go?

@CrossEye CrossEye dismissed buzzdecafe’s stale review January 22, 2022 20:02

Actions already taken, user not active

@CrossEye CrossEye merged commit 049893d into ramda:master Jan 22, 2022
@adispring adispring mentioned this pull request Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants