-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Do not re-raise the exception if the one that we are checking #4761
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha yea, I'm surprised the linter didn't pick that up as effectively doing nothing :/
readthedocs/core/utils/__init__.py
Outdated
@@ -222,6 +222,7 @@ def safe_makedirs(directory_name): | |||
try: | |||
os.makedirs(directory_name) | |||
except OSError as e: | |||
if e.errno == errno.EEXIST: | |||
if e.errno == errno.EEXIST: # 17, FileExistsError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rewrite this as raise if not e.errno == errno.EEXIST
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can't. Doesn't look like valid syntax to me.
>>> raise if None is []
File "<stdin>", line 1
raise if None is []
^
SyntaxError: invalid syntax
>>>
>>> raise if None is [] else 2
File "<stdin>", line 1
raise if None is [] else 2
^
SyntaxError: invalid syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. We should at least be able to do:
if not e.errno == errno.EEXIST:
raise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, maybe we can write it like
if e.errno != errno.EEXIST:
raise
is that what you ment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, even better :D
81c7d44
to
812b666
Compare
👍 |
I think that this commit introduced the problem: 0432ee8 (this is the PR: #2765)
The problem is that it's always re-raising the exception, even if the problem is the one that we checking in the
if
. I think this has generated 4.2k Sentry logs: https://sentry.io/read-the-docs/readthedocs-org/issues/628363742/?query=is:unresolved