I made it work on windows 7 and support Chinese

[AntonioChen]

Hi mk-fg.
First of all, I want to say thank you for your project.
And now, I made it work on windows 7 and support Chinese. So I want to pull my repo to yours.
My English isn't well. And it's my first time to use python and github. So please let me know if there was anything wrong.
Thanks again.

[mk-fg]

Apologies for being a bit unresponsive here, can't seem to dedicate the time to go over the changes, but I expect I will sometime next week.

A few questions I have right off the bat is about commit messages in the first ones in the series - why not describe what you intended to change there instead on "1"?
E.g. just "spaces" or "tabs -> spaces" would've done a good job of making git-log readable.

Other question is related to first one, why "tabs -> spaces"?
I'm not particulary attached to tabs (such things can be handled transparently), but I'm a bit more attached to consistency and commit is a huge "change all lines" blob (see 2bdd531 ).
Is there a particular reason it has to be there? What's the rationale there?
Maybe you can rebase others on current code without that one?

I'll take a look at other changes when I'll get a chance.

[AntonioChen]

Thanks for pointing out my errors. I'm new here, so at the beginning of this, I did something wrong or unreasonable to test something. I'm sorry for these things.
I use pycharm to auto reformat the code. So when you find that a large block of code is deleted and then added again, it's reformatting and you can just skip it. And it's also the reason why "tabs -> spaces". I really do not mean it.
At the end, I think you can skip what I did in every commit and just compare my final version to yours. As I remember, I just edited cli_tool.py and added portalocker.py finally.
Please let me know if I should do something.

[mk-fg]

Just started to merge the stuff and it seem to be easier to just accept "tabs -> spaces" change, so merged it in as well.

It's not really "stupid" - 4-spaces seem to be dominant convention indeed, I just hate extra pointless work merging such spacing changes and resolving conflicts they produce, but hopefully no tool will change that back to tabs.

I don't quite get why later hunks have something like this though:

@@ -192,8 +191,8 @@ class SkyDriveAuth(SkyDriveHTTPClient):
             post_data.update(
                 refresh_token=self.auth_refresh_token, grant_type='refresh_token')
         post_data_missing_keys = list(k for k in
-            ['client_id', 'client_secret', 'code', 'refresh_token', 'grant_type']
-            if k in post_data and not post_data[k])
+                                      ['client_id', 'client_secret', 'code', 'refresh_token', 'grant_type']
+                                      if k in post_data and not post_data[k])
         if post_data_missing_keys:
             raise AuthenticationError('Insufficient authentication'
                                       ' data provided (missing keys: {})'.format(post_data_missing_keys))
@@ -242,7 +241,7 @@ class SkyDriveAPIWrapper(SkyDriveAuth):
             Shouldn't be used directly under most circumstances.'''
         if query_filter:
             query = dict((k, v) for k, v in
-                query.viewitems() if v is not None)
+                         query.viewitems() if v is not None)
         if auth_header:
             request_kwz.setdefault('headers', dict()) \
                 ['Authorization'] = 'Bearer {}'.format(self.auth_access_token)

Can't pycharm format stuff consistently, so that diff and git-log will show what really changed and not this constant space-juggling?

Anyway, easy enough to skip these particular hunks, so guess I'll try to just drop these from non-whitespace commits and merge into purely "code style changes" commits, and suggest you do something similar for the reasons above.

[mk-fg]

8e1c2fa changes "skydrive-cli" in the repo root from symlink (I think these are supported on windows now as well, no?) to just a text file (with destination path inside), didn't apply that, would be great if you'd avoid changing that or maybe turn it into a python wrapper which would import "skydrive.cli_tool" relative to it's path and run the cli.

My reasoning behind that symlink/wrapper is that git clone python-skydrive && cd python-skydrive && ./skydrive-cli ... can work and be more obvious than e.g. python skydrive/cli_tool.py ... in the last step.

[mk-fg]

Hmm, and then there's an introduction of "optz = optz_decode(parser.parse_args())" line, which decodes all CLI arguments to unicode, which can't be right on unixes (e.g. linux) where filesystem path is a bytestring, so if you pass some broken undecipherable bytes, they are valid filename and should never be decoded into anything, as I don't know of a way to to guarantee that a) they will decode b) re-encoding will produce same bytes on python2.

I think python3 introduced some tricky system of decoding paths, leaving undecodable bytes in resulting unicode string, so that passing that string to os.* functions is guaranteed to encode it to the same bytes.

I've heard that fs-stuff returns unicode on windows or something like that, but really have no clue how it works there, I'm afraid.

Anyhow, I haven't really thought much of encodings before, so assume it worked reliably only for ascii on unixes as well, so introduction of at least some encoding-awareness can't be a bad thing, I guess.
If it'll break horribly on unix, at least it'll be some good test-case to fix...

That said, using chardet looks like a horrible way to work with encodings - it never guarantees to get it right, so imagine trusting it to upload 10k files - it might work for 99% of them, but will then break for some.
I'd prefer to have some reliable option like "decode everything as utf-8", so that at least one can be sure that given utf-8 inputs, everything should work reliably.
But as it's optional (eventually), guess there's no harm having it there, if only for ease-of-use in "just want to upload dancing hamster clip!" case.

Guess I'll add an option to "do not detect, always use encoding X", if it's not there.

[mk-fg]

With regard to ca4ada5 - note that there's nice'ish (apart from the fact that iteratng over object attrs is generally a bad practice, of course) vars() builtin that can be used instead of "dir() + getattr()" and "optz.dict" there.

[mk-fg]

2fde2f8 is kinda bad.

That eval() should not work if file has quotes inside and can execute whatever random python code from resulting YAML.
Recent YAML parsing issues are pita to deal with as it is, adding plain eval() of the serialization on top of that is just a crime against sanity ;)

Good way to go about it is to use something that isn't strictly YAML (I wrote this once to print cyrillic chars in the output, among other things), and given simple output types (json can only contain few basic ones), I'd go with some simple hand-rolled indent-and-print() implementation, can you maybe replace eval with that?

[mk-fg]

Disregard the last "can you maybe replace eval with that?" query - as of b33a03d, implemented simple non-yaml (but yaml-like) dumper as follows:

def tree_node(): return defaultdict(tree_node)

def print_result(data, file=sys.stdout, indent='', indent_level=' '*2):
  if isinstance(data, list):
    for v in data:
      print_result(v, file=file, indent=indent + '- ')
  elif isinstance(data, dict):
    for k, v in sorted(data.viewitems(), key=op.itemgetter(0)):
      print(indent + decode_obj(k, force=True) + ':', file=file, end='')
      if not isinstance(v, (list, dict)): # peek to display simple types inline
        print_result(v, file=file, indent=' ')
      else:
        print(file=file)
        print_result(v, file=file, indent=indent+indent_level)
  else:
    print(indent + decode_obj(data, force=True), file=file)

Output should be roughly the same as with YAML but without unicode escapes for non-ascii chars:

Pics:
  image.jpg: photo
  тест:
    фывываыва: file
    фывываыва2: file

I've tested it in utf-8 console with cyrillic (russian), but can you please try with cmd (or powershell, or whatever is currently used there) on windows?
Note that if python fails to autodetect terminal encoding for you (print(u'...non-ascii...') fails), new --encoding option can be used to force all cli i/o to be decoded/encoded with specified codec (e.g. "-e gbk"), but I think some generic non-ascii fallback (e.g. utf-8) might be better for output than the default ascii, if it indeed fails on windows.

Also, it'd be great if you'd be able to check if all the merged stuff in current master branch (in this repo) works.

I've squashed some similar and non-descriptive commits (especially early ones), but otherwise tried to preserve metadata during the merge, so that it might be easier for you to map the merged changesets to the original ones.

If you'll confirm that it works as expected and I haven't missed anything (always a possibility), I'll be able to close this PR and push the new version to pypi.

[AntonioChen]

It seems that I made many mistakes, although I can only understand a bit of what you said because of my poor English. I'll try my best to understand all of these latter. But now, it's more important to test current master branch and I'm working on it. You can get my answer latter.

[AntonioChen]

I found these bugs.

D:\workspace\GitHub\mk-fg\python-skydrive>python .\skydrive\cli_tool.py recent
- comments_count:Traceback (most recent call last):
  File ".\skydrive\cli_tool.py", line 325, in <module>
    if __name__ == '__main__': main()
  File ".\skydrive\cli_tool.py", line 318, in main
    if res is not None: print_result(res, file=sys.stdout)
  File ".\skydrive\cli_tool.py", line 34, in print_result
    print_result(v, file=file, indent=indent + '- ')
  File ".\skydrive\cli_tool.py", line 39, in print_result
    print_result(v, file=file, indent=' ')
  File ".\skydrive\cli_tool.py", line 44, in print_result
    print(indent + decode_obj(data, force=True), file=file)
  File ".\skydrive\cli_tool.py", line 59, in decode_obj
    return obj if not dump else repr(obj)
NameError: global name 'dump' is not defined

'info' is similar.

D:\workspace\GitHub\mk-fg\python-skydrive>python .\skydrive\cli_tool.py mkdir \python
Traceback (most recent call last):
  File ".\skydrive\cli_tool.py", line 325, in <module>
    if __name__ == '__main__': main()
  File ".\skydrive\cli_tool.py", line 276, in main
    metadata=optz.metadata and json.loads(optz.metadata) or dict())
  File "D:\workspace\GitHub\mk-fg\python-skydrive\skydrive\api_v5.py", line 317, in mkdir
    return self(folder_id, data=metadata, method='post', auth_header=True)
  File "D:\workspace\GitHub\mk-fg\python-skydrive\skydrive\api_v5.py", line 254, in __call__
    return self.request(api_url(), **kwz)
  File "D:\workspace\GitHub\mk-fg\python-skydrive\skydrive\api_v5.py", line 121, in request
    raise raise_for.get(code, ProtocolError)(code, err.message)
api_v5.ProtocolError: (400, '400 Client Error: Bad Request')

[mk-fg]

Thanks for testing.

Yeah, fixed the first oversight in 527adb2, changed that option name at the last moment and forgot to update it in the function code.

Second issue seem to be legitimate SkyDrive limitation though - note that you're doing python .\skydrive\cli_tool.py mkdir \python and SkyDrive seem to disallow slashes in names.
Fwiw, skydrive-cli mkdir '\python' seem to fail here as well with the same http-400 error.

[mk-fg]

Hmm, though remembering that '\python' might look like a legitimate "from root" path on windows, guess these slashes can be treated as path separators, same as "/" do.

[mk-fg]

As of 0c67096, should be possible to do:

skydrive-cli mkdir '\python'
skydrive-cli mkdir '\python\subdir'
skydrive-cli mkdir '\python\subdir\test'
skydrive-cli mkdir test/test_subdir python/subdir

First three lines basically just a simplier and more intuitive way than the previous "mkdir name parent_dir" interface.
As shown on the last line, parent_dir can still be used, if necessary.

[AntonioChen]

I like this commit ~
I can't find any bugs now.. It can be closed now..

[mk-fg]

Thanks a lot for your work on porting and testing!