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

Titlise column verbose_name when derived from model #382

Merged
merged 4 commits into from
Sep 9, 2016
Merged

Titlise column verbose_name when derived from model #382

merged 4 commits into from
Sep 9, 2016

Conversation

shawnnapora
Copy link
Contributor

This is my attempt at #249 and #368. If the verbose_name of a model is used to generate the verbose_name of the column, it is titlised. If the verbose_name is passed directly from a Column, it is used verbatim (see the test_column_verbose_name "override" tests). I updated the existing tests to reflect this change and added a couple new tests as well. If there's anything that I overlooked or need to change for this to be acceptable please let me know.

Any verbose_name that was not passed explicitly in the column
definition is returned titlised in keeping with the Django convention
of verbose_names being defined in lowercase and uppercased/titlised
as needed by the application.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add backticks around variable/property names here? i.e.: verbose_names -> verbose_names

@jieter
Copy link
Owner

jieter commented Sep 9, 2016

Apart from some line comments I added, it looks really good, thanks! Will merge after you work on those.

@shawnnapora
Copy link
Contributor Author

I updated the docstring with backticks per your note, and tried to simplify the booleancolumn but it failed a test with my change. See the linenote for details. Let me know if I can do anything else here.

@jieter
Copy link
Owner

jieter commented Sep 9, 2016

I'll merge when you revert the verbose_name=title(... change.

@shawnnapora
Copy link
Contributor Author

Sorry about that, I thought I fixed that in a commit --amend after I caught the test failure. Reverted.

@jieter jieter merged commit d257ee8 into jieter:master Sep 9, 2016
@jieter
Copy link
Owner

jieter commented Sep 9, 2016

Merged, thanks!

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