Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Get completed items #90

Merged
merged 1 commit into from
Aug 15, 2023
Merged

Get completed items #90

merged 1 commit into from
Aug 15, 2023

Conversation

jkawamoto
Copy link
Contributor

It's useful if the client can get completed tasks. This PR implements get_completed_items that calls Get completed items API.

Since it looks like models are migrating to use dataclass instead of attrs (#85), this PR also uses dataclass to define models.

Related to #32 and #56.

@jkawamoto jkawamoto mentioned this pull request Jan 23, 2023
Open
@theptrk
Copy link

theptrk commented Jan 24, 2023

This would be very useful!

I hope this library isn't REST API only since the Sync API still has useful endpoints such as activity logs.

lefcha
lefcha requested changes Aug 9, 2023
View reviewed changes
Copy link
Collaborator

@lefcha lefcha left a comment

Choose a reason for hiding this comment

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

The PR looks great, apart from the one comment I had. If it's addressed we can merge it. Thanks!

todoist_api_python/models.py Outdated Show resolved Hide resolved
@jkawamoto
Copy link
Contributor Author

Thank you for your review. I removed the docstrings and rebased this branch onto the latest main.

@jkawamoto jkawamoto requested a review from lefcha August 10, 2023 20:34
@lefcha lefcha merged commit 8403c89 into Doist:main Aug 15, 2023
1 check passed
@theptrk
Copy link

theptrk commented Aug 15, 2023

thanks for merging this 🎉

@brunorosilva
Copy link

Great! @lefcha, I'm working on #96 which will also use the Sync API, will this be a problem in the future? The doc seems clear about not using calls to the Sync API.

@lefcha
Copy link
Collaborator

lefcha commented Aug 16, 2023

@brunorosilva It's OK to use auxiliary API endpoints from the Sync API in this library. What we don't want is to use the sync command itself. Unfortunately the archiving functionality is part of the sync command, so a PR would not be accepted.

You would need to wait for us to implement this in the REST API. CC @PotHix

@brunorosilva
Copy link

OK @lefcha , thanks for the info.

@jkawamoto jkawamoto deleted the completed_tasks branch June 13, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lefcha lefcha lefcha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jkawamoto @theptrk @brunorosilva @lefcha