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

Replace asserts with conditions #1025

Closed
wants to merge 1 commit into from
Closed

Conversation

4383
Copy link

@4383 4383 commented Dec 21, 2023

Running the python interpreter with the optimization mode turned on (python -O, python -OO, set PYTHONOPTIMIZE=1), will remove all the assert statements from running code [1][2][3].

Dnspython heavily rely on assert to check things at runtime. That could be an issue, if the flag is enabled. All the dnspython check would be removed, leading to possible bugs.

This patch propose to replace assert statements with condition and by raising an AssertionError if the condition is met.

This patch won't change the logic and the behaviour of the checks previously made by using assert, however, these changes will remain actives at runtime, even if python optimization is enabled.

This patch do not replace assert in unit tests because they are not an issue.

[1] https://docs.python.org/3/using/cmdline.html?highlight=pythonoptimize#cmdoption-O
[2] https://docs.python.org/3/using/cmdline.html?highlight=pythonoptimize#cmdoption-OO
[3] https://docs.python.org/3/using/cmdline.html?highlight=pythonoptimize#envvar-PYTHONOPTIMIZE

Running the python interpreter with the optimization mode turned on
(`python -O`, `python -OO`, `set PYTHONOPTIMIZE=1`), will remove
all the assert statements from running code [1][2][3].

Dnspython heavily rely on `assert` to check things at runtime.
That could be an issue, if the flag is enabled. All the dnspython
check would be removed, leading to possible bugs.

This patch propose to replace `assert` statements with condition and
by raising an `AssertionError` if the condition is met.

This patch won't change the logic and the behaviour of the checks
previously made by using `assert`, however, these changes will remain
actives at runtime, even if python optimization is enabled.

This patch do not replace `assert` in unit tests because they are
not an issue.

[1] https://docs.python.org/3/using/cmdline.html?highlight=pythonoptimize#cmdoption-O
[2] https://docs.python.org/3/using/cmdline.html?highlight=pythonoptimize#cmdoption-OO
[3] https://docs.python.org/3/using/cmdline.html?highlight=pythonoptimize#envvar-PYTHONOPTIMIZE
@rthalley
Copy link
Owner

Dnspython is NOT using assert for error checking. Dnspython uses asserts for two basic reasons: 1) to say that something should always be true because of the design of dnspython, and if it isn't there's a bug in dnspython that needs to be fixed, and 2) to give extra help to mypy which can't figure out some things, e.g. we will "assert x is not None" sometimes not because x could be None at that point, but because mypy can't figure out that it can't be None. In both of these cases, dnspython should behave no differently in -O or -OO mode.

@rthalley rthalley closed this Dec 21, 2023
@4383
Copy link
Author

4383 commented Dec 21, 2023

So be it!
Thanks for your feedback.

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