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

Testing Documentation #24

Merged
merged 5 commits into from Feb 19, 2017
Merged

Testing Documentation #24

merged 5 commits into from Feb 19, 2017

Conversation

jackcarlisle
Copy link
Member

ready for review

  • adds information to the README.md on testing with Elixir

#21

@nelsonic nelsonic added the in-review Issue or pull request that is currently being reviewed by the assigned person label Feb 14, 2017
README.md Outdated
#### Pipe Operator. What if we wanted to call some of our functions in succession
to another. Let's create a function that creates a zoo, randomises it and then
returns a selected number of animals to go and see:
#### Pipe Operator. What if we wanted to call some of our functions in succession to another. Let's create a function that creates a zoo, randomises it and then returns a selected number of animals to go and see:
Copy link
Member

Choose a reason for hiding this comment

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

@jackcarlisle I agree with having a sub-section for Pipe Operator but not a fan of the super-long <h4> heading ... could we re-word this?

README.md Outdated
end
end
```
**NOTE: It automatically includes a line called `doctest Animals`. What this means
Copy link
Member

@nelsonic nelsonic Feb 14, 2017

Choose a reason for hiding this comment

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

Does the whole NOTE: ..." need to be bold
or could we make just the "Note" word bold and the rest of the text normal? 🤔

README.md Outdated
end
```

**NOTE: you do not need to install and require any external testing frameworks.
Copy link
Member

Choose a reason for hiding this comment

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

@jackcarlisle same here. NOTE -> Note: ... ?

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

@jackcarlisle this is a superb addition to the learning resource! thanks!
I made a couple of comments on style.
@tomhuhges made a very valid point that our readmes should have more consistent formatting: dwyl/start-here#118
If you have time, please update.
Then assign to @iteles or @jruts for second review. thanks! ❤️

@nelsonic nelsonic added in-progress An issue or pull request that is being worked on by the assigned person and removed in-review Issue or pull request that is currently being reviewed by the assigned person labels Feb 14, 2017
@nelsonic nelsonic assigned jackcarlisle and unassigned nelsonic Feb 14, 2017
@jackcarlisle
Copy link
Member Author

@nelsonic README.md has been re-formatted!

@jackcarlisle jackcarlisle removed their assignment Feb 14, 2017
@jackcarlisle jackcarlisle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Feb 14, 2017
@iteles iteles added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Feb 16, 2017
@iteles
Copy link
Member

iteles commented Feb 16, 2017

Ha, I did NOT review these in the right order.

Copy link
Member

@iteles iteles left a comment

Choose a reason for hiding this comment

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

Another fantastic addition @jackcarlisle!

Some tiny formatting questions and then we're ready to go!

README.md Outdated
@@ -73,6 +85,7 @@ https://www.sitepoint.com/an-interview-with-elixir-creator-jose-valim/
+ What was "_wrong_" with just writing directly in Erlang? read:
http://www.unlimitednovelty.com/2011/07/trouble-with-erlang-or-erlang-is-ghetto.html

***
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 these tend to look a bit weird with the double lines caused by the titles, but not a blocker at all. We don't have these anywhere else so I might suggest we remove them for consistency.

screen shot 2017-02-16 at 18 51 46

README.md Outdated

## Generating your first Elixir project

## *Generating your first Elixir project*
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to italicise this title? Or did you mean to make it a 3 hash title (given no other titles are in italics)?

README.md Outdated
returns a selected number of animals to go and see:
#### Pipe Operator

What if we wanted to call some of our functions in succession to another. Let's create a function that creates a zoo, randomises it and then returns a selected number of animals to go and see:
Copy link
Member

Choose a reason for hiding this comment

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

Think there should be a question mark at the end of the first sentence? Is it not a question?

@iteles iteles assigned jackcarlisle and unassigned iteles and jruts Feb 16, 2017
@iteles iteles removed the in-review Issue or pull request that is currently being reviewed by the assigned person label Feb 16, 2017
@jackcarlisle jackcarlisle assigned iteles and unassigned jackcarlisle Feb 18, 2017
@jackcarlisle
Copy link
Member Author

@iteles changes made!

Copy link
Member

@iteles iteles left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @jackcarlisle ! 👍

@iteles
Copy link
Member

iteles commented Feb 18, 2017

@jruts All yours for review and merge please/thanks 💯

Copy link
Member

@jruts jruts left a comment

Choose a reason for hiding this comment

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

Looks very clear to me 👍

@jruts jruts merged commit 6f41ffe into master Feb 19, 2017
@jruts jruts deleted the testing branch February 19, 2017 22:01
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.

None yet

4 participants