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

'djdt' is not a registered namespace #1405 #1889

Merged
merged 30 commits into from Apr 30, 2024
Merged

Conversation

VeldaKiara
Copy link
Contributor

Description

The error was 'djdt' is not a registered namespace. It occurred due to misconfiguration of the toolbar settings as well as the debug settings.

Fixes # (issue 1405)

  • Added a check to result in an official error
  • Added tests for the check
  • Updated the docs with the right way to configure the toolbar to work with debug without an issue

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@VeldaKiara VeldaKiara self-assigned this Feb 28, 2024
Copy link
Contributor

@elineda elineda left a comment

Choose a reason for hiding this comment

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

The test with make test doesn't work, you need to change the setting on test to change accept test of the debug toolbar

DEBUG_TOOLBAR_CONFIG = {
    # Django's test client sets wsgi.multiprocess to True inappropriately
    "RENDER_PANELS": False,
    "RUNNING_TESTS": False
}

debug_toolbar/apps.py Outdated Show resolved Hide resolved
docs/installation.rst Outdated Show resolved Hide resolved
@VeldaKiara
Copy link
Contributor Author

@elineda @matthiask @tim-schilling could you test it out and let me know if everything is good and any changes or suggestions you have at this point?

@VeldaKiara
Copy link
Contributor Author

@elineda @matthiask @tim-schilling could you test it out and let me know if everything is good and any changes or suggestions you have at this point?

Is it okay to merge?

@tim-schilling
Copy link
Contributor

@VeldaKiara no, we have some miscommunications yet. I started on a change that will better communicate them. I'll try to push that up tonight.

@VeldaKiara
Copy link
Contributor Author

@VeldaKiara no, we have some miscommunications yet. I started on a change that will better communicate them. I'll try to push that up tonight.

Awesome, I will be waiting on the changes.

@tim-schilling
Copy link
Contributor

@VeldaKiara I pushed some changes to the check and the settings in the example app. Let me know what you think.

I suspect we should ignore our new check in the test settings until we test it explicitly. In that case we should use the @override_settings decorator to enable it. That should get the tests to pass.

Regarding the documentation, maybe we keep the docs as they are, then link them to the example app to see how it's setup in an advanced configuration?

@VeldaKiara
Copy link
Contributor Author

Let me check out the changes.

I think that would work to pointing to the example app

@VeldaKiara
Copy link
Contributor Author

I have incorporated the changes requested.
Let me know if you have any more suggestions.

Copy link
Contributor

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

This is coming along nicely Velda. I've got a few suggested changes. Hopefully it doesn't break any of the tests.

tests/test_checks.py Outdated Show resolved Hide resolved
tests/test_checks.py Outdated Show resolved Hide resolved
docs/installation.rst Outdated Show resolved Hide resolved
docs/installation.rst Outdated Show resolved Hide resolved
docs/changes.rst Outdated Show resolved Hide resolved
@VeldaKiara
Copy link
Contributor Author

I have removed the patch decorator, updated the docs and made some changes to the test_panel_empty check for the test to pass

tests/test_checks.py Outdated Show resolved Hide resolved
tests/test_checks.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

Thank you for sticking with this one @VeldaKiara!

@matthiask @elineda @living180 could one of you give this a look over to confirm it makes sense to you? Specifically the naming and documentation.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I mostly have suggestions and questions for clarifications, so I'm approving already.

docs/changes.rst Outdated Show resolved Hide resolved
debug_toolbar/apps.py Outdated Show resolved Hide resolved
tests/settings.py Show resolved Hide resolved
@@ -236,8 +239,43 @@ def test_check_w007_invalid(self, mocked_guess_type):
],
)

def test_debug_toolbar_installed_when_running_tests(self):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you're modifying the dt_settings.get_config() return value instead of using e.g. with override_settings(...)? I suspect that there's a good reason for it; if there is, please add a short docstring.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to use override_settings, we'd have to define the entire DEBUG_TOOLBAR_CONFIG dictionary. And I don't think there's a merge settings method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know what you think of the new comment.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! No further comments :-)

tests/test_checks.py Show resolved Hide resolved
@matthiask
Copy link
Member

I think this is ready to merge, or is there anything left to do?

@tim-schilling tim-schilling merged commit 2f4c471 into jazzband:main Apr 30, 2024
26 checks passed
@VeldaKiara
Copy link
Contributor Author

👯🏾👯🏾👯🏾👯🏾

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

4 participants