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.

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

Merged
merged 4 commits into from
Oct 18, 2023

Conversation

thales-fukuda
Copy link
Contributor

Closes #241

@thales-fukuda thales-fukuda changed the title Add duration fix: Add duration to getTask, addTask, quickAddTask and UpdateTask Oct 13, 2023
@henningmu
Copy link
Contributor

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

  • 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?

Copy link
Contributor

@pawelgrimm pawelgrimm left a 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)

src/types/entities.ts Outdated Show resolved Hide resolved
src/types/requests.ts Outdated Show resolved Hide resolved
src/types/requests.ts Outdated Show resolved Hide resolved
@thales-fukuda
Copy link
Contributor Author

thales-fukuda commented Oct 18, 2023

Thanks for the review @pawelgrimm!
I pushed a commit with the changes, please take a look to see if I didn't miss anything.

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?

Copy link
Contributor

@pawelgrimm pawelgrimm left a 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!

@pawelgrimm pawelgrimm merged commit c64ed8c into Doist:main Oct 18, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task, AddTaskArgs and UpdateTaskArgs types are missing duration
3 participants