Skip to content

Conversation

@willkg
Copy link
Member

@willkg willkg commented Feb 28, 2012

r?

* add line for pep8 so pep8 check shuts up
* remove what looks like dead code
Removes categories 30, 40, and 50 from "untranslated", "immediate
updates needed" and "updates needed" sections and removes counts
from the All Knowledge Base Articles number.

Adds "navigation articles" with category 50 articles. Adds to overview
and sections below.

Adds relevant tests.
Copy link
Member Author

Choose a reason for hiding this comment

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

This removes articles in categories 30, 40 and 50 from the All Knowledge Base Articles total.

@willkg
Copy link
Member Author

willkg commented Feb 28, 2012

One thing that occurs to me while I'm writing comments on my own pull request: it's worth double-checking the SQL. I did some fast-and-loose copy-and-paste, but it's entirely possible that I don't really grok the nature of a "navigation article" and thus have the sql for it wrong. Having said that, it seems right.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer this indentation style so much now :-). Much more predictable.

This one covers all the things that shouldn't be in All Docs
plus it fixes the comment.
@rlr
Copy link
Contributor

rlr commented Feb 28, 2012

I went through and made changes to navigation articles and made sure they didn't show up where they don't belong anymore (they do if I am on master). The new navigation section is nice and worked as expected. Tests passing.

r+ with the minor comments.

@willkg
Copy link
Member Author

willkg commented Feb 28, 2012

I pushed a new unit test to replace the one that had a bad comment. I think that covers it.

Can you do a quick r? on that new unit test?

Copy link
Contributor

Choose a reason for hiding this comment

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

This line and 157 can be deleted :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Bah.

@willkg
Copy link
Member Author

willkg commented Feb 28, 2012

Ok. Those comments are fixed. Good to go?

@rlr
Copy link
Contributor

rlr commented Feb 28, 2012

all good! r+

@willkg
Copy link
Member Author

willkg commented Feb 28, 2012

96a9bf1

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