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

Fixed #16027 -- Added app_label to ContentType.__str__(). #10776

Merged
merged 1 commit into from Feb 8, 2019

Conversation

Projects
None yet
3 participants
@gregschmit
Copy link
Contributor

commented Dec 21, 2018

@gregschmit

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Test result on pr-mariadb/database=mysql_gis,label=mariadb,python=python3.7 shows no failures. I think this commit passes all tests. On my system I had to run this commit off of 2.1.4 to pass tests, but failures off of master seemed unrelated to this commit, as they were the same failures for running master branch unchanged.

@gregschmit

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Just pushed another commit on this ticket since the Permission model was showing the app_label twice after my original change. This fixes that behavior and I ran tests and everything passed.

@gregschmit

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Yay, and now all checks passed.

@timgraham timgraham force-pushed the gregschmit:ticket_16027 branch 2 times, most recently from 118d700 to b884654 Dec 21, 2018

@gregschmit

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

Hey @timgraham, this is my first contribution so I'm having trouble parsing exactly what happened with your pushes/commits. I noticed that you pushed out test cases to my fork, but they don't appear to be pulled into Django. I also noticed that now it's failing a check (but the details of the check don't look like it has anything to do with this change).

Do you need me to do anything?

@timgraham

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

I'll review this later, I just rebased the changes and made some adjustments after 194a4b5. The check failure isn't a problem.

@timgraham timgraham force-pushed the gregschmit:ticket_16027 branch from b884654 to 695da83 Jan 1, 2019

@timgraham

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

An issue I see here is the change in the tests/admin_utils/test_logentry.py test. The admin's recent actions uses ContentType.__str__() so the app label is now prefixed:
screenshot from 2019-01-01 10-07-36
While that looks okay for some app labels, the app label isn't generally a user-facing string. I think we should adjust the template to use the old implementation. In fact, maybe we should leave the ContentType.name unchanged and use that in the template. Then we could add a new ContentType property that includes the app_label and use that in ContentType.__str__().

Please uncheck "patch needs improvement" and "needs tests" on the ticket after updating so that the ticket returns to the review queue.

@gregschmit

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

@timgraham Ok, so I want to make sure I understand you properly:

  1. I should make the .name property just render the name of the model (without the app_label).
  2. I should make the template use the .name property.
  3. I should make a new property, say .qualified_name, that gives the name with the app_label.
  4. I should change the __str__() method to use the .qualified_name.
  5. I should make tests be sane with this layout.

Is that what I understand you mean? Also, would .qualified_name be a sensible property name in your opinion?

@timgraham

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Correct. Maybe call the new property app_labeled_name?

@timgraham timgraham force-pushed the gregschmit:ticket_16027 branch from 695da83 to 1402145 Feb 7, 2019

@timgraham timgraham changed the title Fixed #16027 -- Added app_label to ContentType Fixed #16027 -- Added app_label to ContentType.__str__(). Feb 7, 2019

@timgraham timgraham force-pushed the gregschmit:ticket_16027 branch from 1402145 to 48c1780 Feb 8, 2019

@timgraham timgraham merged commit 48c1780 into django:master Feb 8, 2019

19 checks passed

docs Build #18636 ended
Details
flake8 Build #18746 ended
Details
isort Build #18771 succeeded in 20 sec
Details
pr-mariadb/database=mysql,label=mariadb,python=python3.7 Build #3205 ended
Details
pr-mariadb/database=mysql_gis,label=mariadb,python=python3.7 Build #3205 ended
Details
pull-requests-bionic/database=mysql,label=bionic-pr,python=python3.6 Build #3684 ended
Details
pull-requests-bionic/database=mysql,label=bionic-pr,python=python3.7 Build #3684 ended
Details
pull-requests-bionic/database=mysql_gis,label=bionic-pr,python=python3.6 Build #3684 ended
Details
pull-requests-bionic/database=mysql_gis,label=bionic-pr,python=python3.7 Build #3684 ended
Details
pull-requests-bionic/database=postgis,label=bionic-pr,python=python3.6 Build #3684 ended
Details
pull-requests-bionic/database=postgis,label=bionic-pr,python=python3.7 Build #3684 ended
Details
pull-requests-bionic/database=postgres,label=bionic-pr,python=python3.6 Build #3684 ended
Details
pull-requests-bionic/database=postgres,label=bionic-pr,python=python3.7 Build #3684 ended
Details
pull-requests-bionic/database=spatialite,label=bionic-pr,python=python3.6 Build #3684 ended
Details
pull-requests-bionic/database=spatialite,label=bionic-pr,python=python3.7 Build #3684 ended
Details
pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3.6 Build #3684 ended
Details
pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3.7 Build #3684 ended
Details
pull-requests-javascript Build #15139 ended
Details
pull-requests-windows/database=sqlite3,label=windows,python=Python37 Build #10741 ended
Details
@gregschmit

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

Hey Tim, thanks for making the changes! I was planning on making some time this weekend, but I see you got there first!

-gns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.