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

[bug 784325] Add display_order and is_listed to forums. #806

Closed
wants to merge 2 commits into from

Conversation

rlr
Copy link
Contributor

@rlr rlr commented Aug 30, 2012

  • Also includes a commit to unbreak the admin changelist forms.

r?

* Our required field monkeypatch was triggering invalid validation.
  or something like that
@@ -27,7 +27,8 @@

def forums(request):
"""View all the forums."""
qs = Forum.objects.select_related('last_post', 'last_post__author')
qs = Forum.objects.filter(is_listed=True)
qs = qs.select_related('last_post', 'last_post__author')
Copy link
Member

Choose a reason for hiding this comment

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

Why not combine these?:

    qs = (Forum.objects.filter(is_listed=True).select_related('last_post', 'last_post__author')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will if they fit in the 80 chars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, they don't. Is wrapping in a parens and breaking at the . preferred?

Copy link
Member

Choose a reason for hiding this comment

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

We do this sort of thing in a lot of places in the code:

    qs = (Forum.objects.filter(is_listed=True)
                       .select_related('last_post', 'last_post__author'))

But, "preferred" in this case is really just what we want to do in the Kitsune code. It doesn't really matter much either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had originally broken the line at the first open ( but I didn't like the result. I'll try this way.

@willkg
Copy link
Member

willkg commented Sep 4, 2012

Tests pass. It looks ok to me as far as I can tell. r+

@rlr
Copy link
Contributor Author

rlr commented Sep 4, 2012

landed 6cf5ab8

@rlr rlr closed this Sep 4, 2012
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

2 participants