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

Clarify the function call in "to throw" documentation. #493

Merged
merged 1 commit into from
Jan 9, 2019

Conversation

alexjeffburke
Copy link
Member

This PR hopes to address the concerns in issue #411.

The changes make the wrapping function clear and also include examples using arrow function syntax.

@papandreou
Copy link
Member

Yeah, that's another downside of supporting node 0.10 -- we can't use arrow functions in the documentation code snippets :)

did not throw
```

Used with arrow functions this assertion is very elegant, and there
are a couple of functionally equaivalent aliases provided to allow
Copy link
Member

Choose a reason for hiding this comment

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

equivalent

@alexjeffburke
Copy link
Member Author

alexjeffburke commented Jul 2, 2018

@papandreou check. And re the arrow functions, honestly that's another vote to do as mocha do and go node 4+ in my book :)

@papandreou
Copy link
Member

Or node 6+ while we're at it :)

@alexjeffburke
Copy link
Member Author

Hmm yeah - I only assumed we’d stick with 4 to be in line with mocha.

@sunesimonsen
Copy link
Member

@papandreou isn't the Node versions which we test and generate the documentation deciding if we can use arrow functions? I actually agree. Maybe we should just jump directly to Node v6+

@papandreou
Copy link
Member

@sunesimonsen, not sure I fully understood that? The code snippets in the documentation are executed via mocha as part of make test It doesn't look like we attempt to limit the node versions where the
md files are also included in the test run. We could do that, of course.

@dasilvacontin
Copy link

I can see people thinking "why wrap the function pointlessly when I can just pass the function and boom will be called for me".

@dasilvacontin
Copy link

But yeah, that's for people who understand that you have to let the library call the function for you.

How passing functions work is something that is easy mislearned, from what I have seen helping people with JavaScript. Trying to pass callbacks while you are actually executing them in the spot and passing whatever they return, etc.

@alexjeffburke
Copy link
Member Author

@dasilvacontin still very glad you reported it so we get a chance to hopefully improve things :)

@alexjeffburke alexjeffburke changed the base branch from master to v11 September 6, 2018 08:35
@alexjeffburke
Copy link
Member Author

alexjeffburke commented Sep 6, 2018

Quick note, I've rebased this on top of v11 which has addressed the test failures due to the use of arrow functions :)

Copy link
Member

@papandreou papandreou left a comment

Choose a reason for hiding this comment

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

🚀

@papandreou
Copy link
Member

@alexjeffburke, is there anything holding this back, or should we get it into v11?

@alexjeffburke
Copy link
Member Author

@papandreou nothing from my side so yeah let's fold this in :)

@alexjeffburke alexjeffburke reopened this Jan 4, 2019
@sunesimonsen sunesimonsen changed the base branch from v11 to master January 5, 2019 23:23
@papandreou
Copy link
Member

I keep stumbling upon this one, let's just get it in :)

@papandreou papandreou merged commit ea0a49d into master Jan 9, 2019
@papandreou papandreou deleted the docs/clarify-to-throw branch January 9, 2019 21:44
@alexjeffburke
Copy link
Member Author

Yay!

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.

4 participants