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

docs(addAssertion): document alternations, flags and optional values #668

Merged
merged 10 commits into from Dec 2, 2019

Conversation

@joelmukuthu
Copy link
Member

joelmukuthu commented Nov 20, 2019

No description provided.

@joelmukuthu joelmukuthu force-pushed the docs/addAssertion branch from d368111 to 0a6a3ad Nov 20, 2019
@sunesimonsen

This comment has been minimized.

Copy link
Member

sunesimonsen commented Nov 20, 2019

Thanks for improving this, I'll review this soon.

@sunesimonsen

This comment has been minimized.

Copy link
Member

sunesimonsen commented Nov 20, 2019

I just read through the updated documentation and I think it is a great improvement. So when we get the tests passing and @papandreou blessing then I think we should run with it.

Copy link
Member

papandreou left a comment

Great initiative, makes it much easier to understand! 🚀

documentation/api/addAssertion.md Outdated Show resolved Hide resolved
documentation/api/addAssertion.md Outdated Show resolved Hide resolved
@joelmukuthu joelmukuthu force-pushed the docs/addAssertion branch from 0b2ea68 to a7dfe1c Nov 21, 2019
@joelmukuthu

This comment has been minimized.

Copy link
Member Author

joelmukuthu commented Nov 21, 2019

Thank you for the review @papandreou and @sunesimonsen. This is ready for a second pass 👼

@alexjeffburke

This comment has been minimized.

Copy link
Member

alexjeffburke commented Nov 21, 2019

Fwiw I also think this is awesome, thanks for taking this on @joelmukuthu 🥇 Have left a small comment inline and will do another top-to-bottom read hopefully this eve.

@joelmukuthu joelmukuthu assigned papandreou and unassigned papandreou Nov 21, 2019
Copy link
Member

alexjeffburke left a comment

I’ve now had the chance to read the text top to bottom - again, really cool taking on docs :) I’ve left a few specific comments inline, but I still think we need to address the “handler” thing.

My suggestion now is to avoid the use of the term handler. I think this could be achieved by saying early on that:

“The function we supply as the second argument to addAssertion is a handler that defines the body of the assertion..” and then later in the document refer to “assertion body”. That make use of terms we’ve generally used for this, but also avoids what to me is a very overloaded word in node.

Finally, I think somewhere we do need to mention that if a promise is returned inside the assertion body that unexpected will wait for it allowing support for asynchronous assertions.

documentation/api/addAssertion.md Outdated Show resolved Hide resolved
documentation/api/addAssertion.md Outdated Show resolved Hide resolved
documentation/api/addAssertion.md Show resolved Hide resolved
@joelmukuthu

This comment has been minimized.

Copy link
Member Author

joelmukuthu commented Nov 26, 2019

Thanks for the review @alexjeffburke. I've replied to some of your comments seeking clarification.

Regarding async assertions, there's a section further down on the same page talking about it. My idea with this rewrite was to build up from a simple example and avoid swamping the reader with too much information in one go.

Regarding the word "handler", just to be sure, you'd like for me to use "assertion body" in its place instead?

@joelmukuthu

This comment has been minimized.

Copy link
Member Author

joelmukuthu commented Nov 26, 2019

FYI, the "fun with flags" reference https://www.youtube.com/watch?v=LeyofQK6tRw :)

EDIT: Actually this, where it all began https://www.youtube.com/watch?v=Xl12Sp1KiEk

Copy link
Member

alexjeffburke left a comment

I really like this version 👍 @sunesimonsen @papandreou all my small notes have been addressed and I'd be happy to proceed.

Copy link
Member

papandreou left a comment

Great! 🚀

The node.js 6 build is failing because of the use of Object.values in one of the examples.

@joelmukuthu

This comment has been minimized.

Copy link
Member Author

joelmukuthu commented Dec 2, 2019

Great, thanks for the reviews! I slinkied my way around the Node 6 build and will squash and merge after a successful build.

@joelmukuthu joelmukuthu merged commit 9c35e90 into master Dec 2, 2019
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 97.973%
Details
@joelmukuthu joelmukuthu deleted the docs/addAssertion branch Dec 2, 2019
@joelmukuthu

This comment has been minimized.

Copy link
Member Author

joelmukuthu commented Dec 2, 2019

Someone remind me, how do I update the docs site? Or anyone mind doing it?

@papandreou

This comment has been minimized.

Copy link
Member

papandreou commented Dec 2, 2019

make deploy-site 🤗

@joelmukuthu

This comment has been minimized.

Copy link
Member Author

joelmukuthu commented Dec 2, 2019

Thank you @papandreou. All published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.