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

Support for grouping fields in admin #145

Merged
merged 1 commit into from
May 31, 2016

Conversation

remik
Copy link
Collaborator

@remik remik commented May 29, 2016

No description provided.

@coveralls
Copy link

coveralls commented May 29, 2016

Coverage Status

Coverage decreased (-0.05%) to 90.909% when pulling bc20b22 on remik:feature/grouping-fields-admin into c427d17 on batiste:master.

@remik
Copy link
Collaborator Author

remik commented May 29, 2016

Please review.
It might need a bit better docs.

Hi from PyCon US 2016 :)

if bit == 'in':
quotes = ('"', "'")
if remaining[1].startswith(quotes) and remaining[1].endswith(quotes):
remaining[1] = remaining[1][1:-1]
Copy link
Owner

Choose a reason for hiding this comment

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

You should use unescape_string_literal here

https://docs.djangoproject.com/en/1.9/_modules/django/utils/text/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed. Thanks!

@batiste
Copy link
Owner

batiste commented May 30, 2016

Just a little comment otherwise this seems good to go! Great feature implemented rather simply.

@batiste
Copy link
Owner

batiste commented May 30, 2016

Also what about being a little more explicit and use "group" or "groupby" as a keyword instead of "in"?

@remik
Copy link
Collaborator Author

remik commented May 30, 2016

I was thinking about "group by" but then it complicates parser code to get more tokens.
For me "group" looks weird but I can change if you want.
For "groupby" it weird - I would put space very often:)
Finally maybe "group_by" ?

@coveralls
Copy link

coveralls commented May 30, 2016

Coverage Status

Coverage decreased (-0.07%) to 90.895% when pulling 902a485 on remik:feature/grouping-fields-admin into c427d17 on batiste:master.

@batiste
Copy link
Owner

batiste commented May 30, 2016

Django template don't seem to use underscore: e.g. truncatewords, filesizeformat, intcomma so I would not go for group_by. What do you think of section? That seems to describe things very well?

@remik
Copy link
Collaborator Author

remik commented May 31, 2016

Yep, "section" is good. I will use it and rebase commits so we will have one history only:)

@remik remik force-pushed the feature/grouping-fields-admin branch from 902a485 to 0d6d1d4 Compare May 31, 2016 00:35
@remik
Copy link
Collaborator Author

remik commented May 31, 2016

I know I can merge it but I will give you time for last check:)

@coveralls
Copy link

coveralls commented May 31, 2016

Coverage Status

Coverage decreased (-0.07%) to 90.895% when pulling 0d6d1d4 on remik:feature/grouping-fields-admin into c427d17 on batiste:master.

@batiste batiste merged commit c91022b into batiste:master May 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants