Repository navigation
Conversation
|
Thank you for your contribution to Django! This pull request has one or more items that need attention before it can be accepted for review.
|
1 similar comment
|
Thank you for your contribution to Django! This pull request has one or more items that need attention before it can be accepted for review.
|
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
…admindocs. Extracted function `_is_callback`, `lookup_str` and attribute `_callback_strs` from UrlResolver to be closer to use inside admindocs.
72b92f2 to
2eda768
Compare
blighj
left a comment
There was a problem hiding this comment.
Thanks for the interest in contributing to django. Welcome aboard.
This looks like the right approach to me, compared to the earlier PR that had to be reverted.
The tests that prove it doesn't regress don't look correct though, I think the admindocs before admin test is not needed. We could test the threading regression with something like:
def test_is_callback_in_thread(self):
"""
_is_callback() works on threads other than the one that imported
django.contrib.admindocs.utils (#33955).
"""
resolver = get_resolver("admin_docs.urls")
results = []
thread = threading.Thread(
target=lambda: results.append(
_is_callback("admin_docs.views.XViewClass", resolver)
)
)
thread.start()
thread.join()
self.assertEqual(results, [True])
What do you think?
| @override_settings( | ||
| INSTALLED_APPS=["django.contrib.admindocs", *SOME_INSTALLED_APPS] | ||
| ) | ||
| def test_valid_order_between_admin_docs_and_admin(self): |
There was a problem hiding this comment.
Do we need this test? it passes without the change and passes on the old 4.1 branch with the fix that needed reverting, so it is not testing anything useful, the doc string is quoting 33955 twice. The quickest fix is to remove it if we don't need it.
| * The undocumented ``URLResolver._is_callback()``, | ||
| ``URLResolver._callback_strs``, and ``URLPattern.lookup_str()`` are | ||
| moved to ``django.contrib.admindocs.utils``. | ||
|
|
There was a problem hiding this comment.
This is the exact same as the original release note in 4.1, which was never removed and no other note never said it was reverted. It probably needs something to say it had been reverted, or maybe we could remove it now from 4.1.txt?
ticket-32565
Branch description
Extracted function
UrlResolver._is_callback,UrlResolver.lookup_strand attributeUrlResolver._callback_strsfrom UrlResolver to be closer to use inside admindocs.AI Assistance Disclosure (REQUIRED)
I used Claude Opus 4.7 to create a base of test that I had almost completely rewrite and do a first review of my changes.
I have manually reviewed, edited, and verified all code and documentation.
Checklist
mainbranch.