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

Fix TODO for tour exercise #108

Closed
wants to merge 3 commits into from
Closed

Conversation

llvieira
Copy link
Contributor

@llvieira llvieira commented Nov 6, 2017

Closes #49. This pull request fix TODO for tour exercise putting a more clear message.

@@ -198,8 +198,7 @@ u:http://oldnews.com/a23 uri hash a6c4d1f

Now that you have data loading, create an observer that watches the reference
status column. This observer should increment word counts when new content is
referenced and decrement word counts when content is dereferenced. The
observer should also delete the content when its dereferenced.
referenced and decrement word counts when content is dereferenced. The observer should also delete the content if zero URLs reference this one and when content's URL reference counts goes to zero, if it was processed before then its word counts should be decremented before deleting.
Copy link
Member

Choose a reason for hiding this comment

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

This new sentence is a bit of a run-on. Perhaps end the sentence after zero, and the rest rephrased to be a second sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing, @ctubbsii. Now I tried to write a bit more clear.

@@ -198,8 +198,7 @@ u:http://oldnews.com/a23 uri hash a6c4d1f

Now that you have data loading, create an observer that watches the reference
status column. This observer should increment word counts when new content is
referenced and decrement word counts when content is dereferenced. The
observer should also delete the content when its dereferenced.
referenced and decrement word counts when content is dereferenced. The observer should also delete the content if zero URLs reference this one. If content's URL reference counts goes to zero and it was processed before, then its word counts should be decremented before deleting.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have been trying to think of way to make this prose easier to understand and I could not. I think replacing the prose with a table would make this easier to understand. What do you think of the following?

Now that you have data loading, create an observer that watches the reference
status column (*doc:refs*).  This observer should take the following actions 
based on the values of the columns *doc:refs* and *doc:processed*.  

| doc:refs     | doc:processed       | Action
| ------------ | ------------------- | ----------
| referenced   | false               | Increment word counts and set *doc:processed* to *true*
| referenced   | true                | None
| unreferenced | false               | Delete content
| unreferenced | true                | Delete content and decrement word counts

@llvieira this is not the changes I was thinking of when I opened that issue (I'll update the issue to clarify). However that issue I opened was awful, it took me a while to figure out what I was thinking. Sorry! Anyway the changes you are making here are great because this is really confusing and needs to be improved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keith-turner I think this table is awesome. I will update the exercise-1.md.

@keith-turner
Copy link
Contributor

Merged in d826227

@llvieira llvieira deleted the fix_todo_tour branch November 8, 2017 23:07
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.

Fix TODO for tour excercise one
3 participants