-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
fix: Add duration to getTask, addTask, quickAddTask and UpdateTask #242
Conversation
@thales-fukuda thanks a lot for getting this done, it works perfectly 🎊 @pawelgrimm has a little bit more context on the duration feature, so I'd like his opinion before releasing this:
Should we mimic these conditions in the SDK as well, or allow everything the API allows? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @thales-fukuda! I left some small comments for you — let me know if you'd prefer I apply the proposed changes myself.
@pawelgrimm has a little bit more context on the duration feature, so I'd like his opinion before releasing this:
- The API only accepts durations when a due date is set; otherwise, it silently discards the durations
- The app is only displaying durations for tasks with due time and durations < 24h
Should we mimic these conditions in the SDK as well, or allow everything the API allows?
Let's keep the SDK aligned with the API — the only requirements are that both (or neither) the unit and amount must be provided and that the amount is positive.
You're welcome to set any duration you'd like, but keep in mind that Todoist won't display the duration in the apps if:
- The task has no due date
- The duration unit is "day"
- The duration unit "minute" and the amount is >=1440 (24 h)
Thanks for the review @pawelgrimm! Also, I was reviewing the SDK types to see if something else is not aligned with the API documentation and I found some stuff. Should I open a new issue or can we discuss it here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 🚀
Also, I was reviewing the SDK types to see if something else is not aligned with the API documentation and I found some stuff. Should I open a new issue or can we discuss it here?
Let's keep this PR atomic and discuss in a new issue. Feel free to tag me directly!
Closes #241