EnterpriseyIntranet / nextcloud-API

NextCloud OCS API for Python
GNU General Public License v3.0
27 stars 27 forks source link

Fixed issue when moving files to path with non latin characters #34

Closed simfr24 closed 5 years ago

simfr24 commented 5 years ago

Added explicit utf-8 encoding to prevent requests from using latin-1

codecov-io commented 5 years ago

Codecov Report

Merging #34 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #34   +/-   ##
=======================================
  Coverage   92.63%   92.63%           
=======================================
  Files          17       17           
  Lines         516      516           
=======================================
  Hits          478      478           
  Misses         38       38
Impacted Files Coverage Δ
src/nextcloud/requester.py 96.29% <ø> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1104a57...4b67653. Read the comment docs.

matejak commented 5 years ago

Thank you for your pull request, would you be OK to submit a test for this? We have something at https://github.com/EnterpriseyIntranet/nextcloud-API/blob/master/tests/test_webdav.py#L160, it should be enough to just change the filename. Next, you encode the data, so it is treated as bytes, but doesn't it mean that you will have to decode it later accordingly? (I am not sure, just asking.)

simfr24 commented 5 years ago

Next, you encode the data, so it is treated as bytes, but doesn't it mean that you will have to decode it later accordingly? (I am not sure, just asking.)

Not really, at least it work in my code without any other modification. I think that when the path is explicitely encoded in utf-8, it is treated like it by requests, but it otherwise behave normally.

simfr24 commented 5 years ago

Thank you for your pull request, would you be OK to submit a test for this? We have something at https://github.com/EnterpriseyIntranet/nextcloud-API/blob/master/tests/test_webdav.py#L160, it should be enough to just change the filename.

Do you mean create another test with a non latin-1 char or just add a non latin-1 char in the current test ? ( Sorry, not exactly an expert 😅)

However I only treated the destination path (as it was only that that failed in my code), not the origin path that will probably fail as well.

matejak commented 5 years ago

Modifying the test as you have suggested is enough. If some errors pop up, I am sure that you will be able to fix them easily and be really proud about your PR.

simfr24 commented 5 years ago

I'm willing to help, but I really don't know how to proceed 😅

I tested on my real NC with a filename full of emojis, and it worked. But I don't undestand how this test framework works, notably : how to actually run the test.

Do you basically just ask me to add emojis in every filename in https://github.com/EnterpriseyIntranet/nextcloud-API/blob/master/tests/test_webdav.py#L160 and push ?

scrutinizer-notifier commented 5 years ago

The inspection completed: No new issues

matejak commented 5 years ago

The first filename should be good enough, and yes, this is the cheap, but perfectly fine way forward. You can then use git rebase -i to merge commits and git push --force-with-lease to push to maintain clean history: https://developer.atlassian.com/blog/2015/04/force-with-lease/ You should be able to run tests easily if you a Linux workstation with Docker. But if you are not familiar with docker-compose, I suggest going for a push/wait for the result approach.

matejak commented 5 years ago

Anyway, you did exactly that. Thank you for your pull request and for the test, great job!

simfr24 commented 5 years ago

Thanks for your help and patience :)