Skip to content
This repository has been archived by the owner on Feb 21, 2022. It is now read-only.

V8 #55

Merged
merged 12 commits into from
Apr 19, 2019
Merged

V8 #55

merged 12 commits into from
Apr 19, 2019

Conversation

lefcha
Copy link
Contributor

@lefcha lefcha commented Feb 14, 2019

No description provided.

@PotHix
Copy link
Member

PotHix commented Mar 19, 2019

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
Copy link
Contributor Author

lefcha commented Mar 22, 2019

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
Copy link
Contributor Author

lefcha commented Mar 22, 2019

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
Copy link
Member

PotHix commented Mar 22, 2019

@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. 👍

@lefcha
Copy link
Contributor Author

lefcha commented Mar 22, 2019

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
Copy link
Contributor Author

lefcha commented Mar 22, 2019

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
Copy link
Member

PotHix commented Mar 22, 2019

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
Copy link
Contributor Author

lefcha commented Mar 22, 2019

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
Copy link
Contributor Author

lefcha commented Apr 15, 2019

@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
Copy link
Contributor Author

lefcha commented Apr 15, 2019

Copy link
Member

@PotHix PotHix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some small comments but it looks good overall! :)

todoist/managers/projects.py Show resolved Hide resolved
CHANGES.md Outdated
@@ -0,0 +1,79 @@
# Changes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call it CHANGELOG instead of CHANGES and also change the file name to reflect it?

CHANGES.md Outdated
@@ -0,0 +1,79 @@
# Changes

Here follows a list of the changes between `v7` and `v8` API versions:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use just the version number as it will be the CHANGELOG file. We can use that for future changes as well. It is something I had I my tasks for a while now.

@lefcha
Copy link
Contributor Author

lefcha commented Apr 18, 2019

OK, the last 3 commits should:

@PotHix
Copy link
Member

PotHix commented Apr 18, 2019

@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
Copy link
Contributor Author

lefcha commented Apr 19, 2019

@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.

@lefcha lefcha merged commit 29864f6 into master Apr 19, 2019
@lefcha lefcha deleted the v8 branch April 19, 2019 08:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants