Doist / todoist-python

DEPRECATED The official Todoist Python API library
MIT License
534 stars 73 forks source link

V8 #55

Closed lefcha closed 5 years ago

PotHix commented 5 years ago

I still have this error sometimes:

SyncError: (u'b3b3159e-4a9b-11e9-aa71-34e12d1f7e35', {u'error_extra': {}, u'error': u'Invalid temporary id', u'error_tag': u'INVALID_TEMPID', u'http_code': 400, u'error_code': 16})

I had three of them when running on Python 2.7 but none when running on Python 3.

The test_share_accept is also failing for me:

>       assert api2.state['user']['id'] in \
            [p['user_id'] for p in api2.state['collaborator_states']]
E       assert 1 in []

Other than that, the code changes look good. I still want to check if all the changes in v8 are covered but it's hard since we don't have a migration guide yet.

We should not release a new version without writing a migration guide for the library as well. At least considering the major changes that will break existing code.

lefcha commented 5 years ago

Is this reproducible? I can't get it to fail for me... I always get 47 passed in 21.45 seconds however times I run it...

If you delete ~/.todoist-sync/ and run again like pytest tests does it happen?

lefcha commented 5 years ago

We should not release a new version without writing a migration guide for the library as well. At least considering the major changes that will break existing code.

OK, so we'll release that when we have the migration guide.

PotHix commented 5 years ago

@lefcha just to be clear: there are two migration guides.

One of them is the official documentation guide for the API. The second one is the migration guide for the Python library. A review/rewrite of the readme + a new section of migration from the new version would be enough. :+1:

lefcha commented 5 years ago

Hm, I didn't think of a Python migration guide, as I don't remember doing one that between 6->7.

Basically I thought that the API doc is enough as it describes all the changes in the API, and those changes are then reflected on the way you call our API using curl or through the Python lib. The examples are also updated to reflect the changed python functions (or the new ones).

lefcha commented 5 years ago

BTW, who is supposed to write the API doc changes between v7/v8 section? Roman, or was it handed over to someone (maybe even me and I don't remember)?

PotHix commented 5 years ago

Hm, I didn't think of a Python migration guide, as I don't remember doing one that between 6->7.

Well, it will help everyone that has a working code with the current version. Writing a migration guide will point to the changes the developer has to look into and he can refer to the documentation for that. If we don't write one, it will be trial and error for the developer.

BTW, who is supposed to write the API doc changes between v7/v8 section? Roman, or was it handed over to someone (maybe even me and I don't remember)?

If I remember correctly, it was part of this task. Migrate the code for v8 and document it.

lefcha commented 5 years ago

If I remember correctly, it was part of this task. Migrate the code for v8 and document it.

I see, I'll fill in the missing changelog and also mention in somewhere in this lib what changed in the python code.

lefcha commented 5 years ago

@PotHix

OK, just added a new kinda changelog file, that documents all the changes between v7 and v8 specifically for the python module.

I also found a few stuff missing or not migrated, so I fixed those too.

lefcha commented 5 years ago

Here's the changelog: https://github.com/Doist/todoist-python/blob/v8/CHANGES.md

lefcha commented 5 years ago

OK, the last 3 commits should:

PotHix commented 5 years ago

@lefcha The content of the CHANGELOG is still the same, it still mention "Changes". Not sure if something was left behind.

Other than that, we should merge this. :)

lefcha commented 5 years ago

@lefcha The content of the CHANGELOG is still the same, it still mention "Changes". Not sure if something was left behind.

Other than that, we should merge this. :)

Yes, you're right the changes were there in my local repo; for some reason I missed including them.