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

Documentation #8

Merged
merged 8 commits into from
Apr 9, 2016
Merged

Documentation #8

merged 8 commits into from
Apr 9, 2016

Conversation

Nemo157
Copy link
Owner

@Nemo157 Nemo157 commented Apr 9, 2016

Adds documentation for all public items and enables the warning for missing it. Had to add an implementation of Eq to Token to make the doc tests for that easy to write, but I think it makes sense to have it; as far as I can tell the only way it can sort of fail is if you have duplicated attributes in different orders.

/// exists in the set will update its value, otherwise will add a new
/// attribute to the set.
/// exists in the list will update its value, otherwise will add a new
/// attribute to the list.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this sounds better: If the attribute already exists in the list, update its value, otherwise add a new attribute to the list..

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

@critiqjo
Copy link
Collaborator

critiqjo commented Apr 9, 2016

Except for those minor corrections, it looks very good! 👍

@critiqjo
Copy link
Collaborator

critiqjo commented Apr 9, 2016

If you're done with docs, I'll try the 'Update branch' button and merge this...

@Nemo157
Copy link
Owner Author

Nemo157 commented Apr 9, 2016

👍

@critiqjo
Copy link
Collaborator

critiqjo commented Apr 9, 2016

Hmm... A reverse merge! Now there'll be another merge commit!

@Nemo157
Copy link
Owner Author

Nemo157 commented Apr 9, 2016

Hmm, yeah, I prefer GitLab's "rebase branch". I could rebase and force push this if you want?

@critiqjo
Copy link
Collaborator

critiqjo commented Apr 9, 2016

I could rebase and force push this if you want?

Alright! I was almost at the merge button!!

@critiqjo critiqjo merged commit 49de63f into master Apr 9, 2016
@Nemo157 Nemo157 deleted the documentation branch April 9, 2016 14:55
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