-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix type errors in locks.py #27
Fix type errors in locks.py #27
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.
Thanks for this charge, it looks like a good fix. I've got a couple of comments / suggestions but they're not necessarily important changes, let me know what you think.
pywebdav/lib/locks.py
Outdated
@@ -153,8 +153,8 @@ def do_LOCK(self): | |||
lock.setTimeout(timeout) # automatically refreshes | |||
found = 1 | |||
|
|||
self.send_body(lock.asXML(), | |||
'200', 'OK', 'OK', 'text/xml; encoding="utf-8"') | |||
self.send_body(str(lock.asXML()).encode('utf-8'), |
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.
lock.asXML already returns a str so wrapping it to then call .encode isn't really necessary, however just calling .encode on the result feels a bit fragile / easy to break in future...
Alternatively, instead of str(lock.asXML()).encode('utf-8')
it can be written as bytes(lock.asXML(), 'utf-8')
though in the end it basically does the same thing.
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 used str(...)
to help PyCharm with the types. I will change it to bytes(...)
like you suggested, it looks better.
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.
Yeah I only recently learnt this usage of bytes() where it does then encode for you if you pass the format. str() has the same thing for going the other direction.
I personally think it's far more description; "I want to convert this object into bytes" rather than just "encode() this object". Either way in this case it's shorter and pycharm will still recognise it :-)
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.
That being said, did you also test that this still works? It wouldn't be the first time I've given incorrect advice and broken something ;-)
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.
After the change to bytes(...)
I have run a couple of random manual tests:
- Mounting via macOS Finder works.
- Create a file via
touch
works. - Edit the same file via
nano
and TextEdit works. - Rename the file works.
- Delete the file works.
- Copy a folder with ~1000 small files from another drive works.
Working with that folder with 1000 files is really slow but I don't think that is related to my changes. The copying went really fast for the first few 100s of files but then it slowed down, paused for a while, copied a few more, paused, etc. Could perhaps be a memory leak issue?
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.
Sorry I can't really comment on the performance issue, I haven't used this myself for some years.
Thanks for your changes and testing notes! My apologies for leaving you hanging on the last comment, I got rather busy at home; baby boy arrived!
Your changes look good and ready to merge. Thanks!
The code in locks.py called WebDAWServer.send_body(DATA, code) with strings passed as "DATA" and "code" parameters while the underlying code expected "DATA" to be a bytes and "code" to be an int. See issue andrewleech#26.
e50715c
to
a0545a1
Compare
Included in https://pypi.org/project/PyWebDAV3/0.10.0/ |
The code in
locks.py
calledWebDAWServer.send_body(DATA, code)
with strings passed asDATA
andcode
parameters while the underlying code expectedDATA
to be abytes
andcode
to be anint
.This should fix issue #26. I tested it with Finder on macOS.
Please note that I only tested this fix on Python 3. Could be that it won't work on Python 2. Let me know if I should test that.