Skip to content

Conversation

@rlr
Copy link
Contributor

@rlr rlr commented Jun 17, 2013

Swithed to use our existing topics_for() helper for listing subtopics.

r?

Swithed to use our existing topics_for() helper for listing subtopics.
@rlr
Copy link
Contributor Author

rlr commented Jun 17, 2013

nevermind OK! 1484 tests, 0 failures, 0 errors, 5 skips in 155.4s

R+! woot

@rlr
Copy link
Contributor Author

rlr commented Jun 17, 2013

oh no wrong PR lol

@rlr
Copy link
Contributor Author

rlr commented Jun 17, 2013

r- @rlr r-

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change to the signature is a little odd because the parent and include_subtopics parameters are really interconnected. It doesn't make a lot of sense to speicify both, but there is nothing stopping you. Sadly I can't think of a way to improve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe default parent to False? and set it if it isn't False. Then kill include_subtopics?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that makes sense. I was trying to think of something like that and getting caught up because my usual null value, None, was already a thing.

@rlr
Copy link
Contributor Author

rlr commented Jun 17, 2013

ok, i like that better ^^

@mythmon
Copy link
Contributor

mythmon commented Jun 17, 2013

Nice! 😀 r+

@rlr
Copy link
Contributor Author

rlr commented Jun 17, 2013

1df094f

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