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

Refs #32508 -- Raised Type/ValueError instead of using "assert" in django.core. #14135

Merged
merged 1 commit into from
Mar 19, 2021

Conversation

abbasidaniyal
Copy link
Contributor

No description provided.

@felixxm
Copy link
Member

felixxm commented Mar 16, 2021

@abbasidaniyal Thanks 👍 Can you also audit other uses of assert in django/core?:

  • django/core/mail/message.py: assert content is None
  • django/core/mail/message.py: assert mimetype is None
  • django/core/mail/message.py: assert content is not None
  • django/core/mail/message.py: assert content is not None
  • django/core/mail/message.py: assert mimetype is not None
  • django/core/files/storage.py: assert name, "The name argument is not allowed to be empty."

and add tests for uncovered error messages.

@abbasidaniyal
Copy link
Contributor Author

@abbasidaniyal Thanks 👍 Can you also audit other uses of assert in django/core?:

  • django/core/mail/message.py: assert content is None
  • django/core/mail/message.py: assert mimetype is None
  • django/core/mail/message.py: assert content is not None
  • django/core/mail/message.py: assert content is not None
  • django/core/mail/message.py: assert mimetype is not None
  • django/core/files/storage.py: assert name, "The name argument is not allowed to be empty."

and add tests for uncovered error messages.

Sure! I was thinking of adding the tests too. Didn't find any previously hence skipped it.
On it now!

@abbasidaniyal
Copy link
Contributor Author

@felixxm It seems as if CheckMessage and it's sub classes (Debug, Info etc) do not have any tests.
I was planning on creating a new file unders tests/check_framework named test_messages.py. Let me know if that's alright.

In the meantime, I'm working on the other instances of assert in django.core.

@felixxm
Copy link
Member

felixxm commented Mar 17, 2021

@felixxm It seems as if CheckMessage and it's sub classes (Debug, Info etc) do not have any tests.

You can add a test to the check_framework.tests.MessageTests.

@abbasidaniyal abbasidaniyal changed the title Refs #32508 -- Raised TypeError instead of using "assert" in CheckMessage and CheckRegistry. Refs #32508 -- Raised TypeError instead of using "assert" in django.core. Mar 17, 2021
@felixxm felixxm self-assigned this Mar 18, 2021
@felixxm felixxm changed the title Refs #32508 -- Raised TypeError instead of using "assert" in django.core. Refs #32508 -- Raised Type/ValueError instead of using "assert" in django.core. Mar 19, 2021
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@abbasidaniyal Thanks 👍

I pushed edits, simplified tests, and added small elease notes.

django/core/mail/message.py Outdated Show resolved Hide resolved
django/core/mail/message.py Outdated Show resolved Hide resolved
tests/mail/tests.py Outdated Show resolved Hide resolved
@felixxm felixxm merged commit 474cc42 into django:main Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants