-
Notifications
You must be signed in to change notification settings - Fork 42
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
Transactional rename on Windows #355
Conversation
1cbb633
to
cadaa7e
Compare
client/sources/ok_test/models.py
Outdated
success = True | ||
except (NotImplementedError, OSError): | ||
pass | ||
if not success: |
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.
Would it make sense for this block to be inside the body of the except
clause and the success = False; success = True
bit go away.
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.
Sure I could do that if you'd like. The reason behind this was that I've gotten burned before in Python 2 when raising exceptions from except
blocks, so I try to avoid it just in case it becomes an issue again (not sure if it's entirely fixed in Python 3 but I try to write code that'd work in Python 2 too).
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-client
should be python3 only, but I'm curious about that issue. There are issues raising different exceptions in an except block in python2?
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.
Yup! What would Python print? :-)
try:
try: raise ValueError("ValueError")
except:
try: raise NotImplementedError("NotImplementedError")
except: pass
raise
except Exception as ex: print(ex)
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.
Okay I just removed success
, see the updated code.
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 I just realized I don't raise NotImplementedError
properly... let me fix that.
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 fixed.
f2955d6
to
6dda3d6
Compare
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.
LGTM
No description provided.