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

VTODO - start, end, duration #733

Merged
merged 9 commits into from
Oct 26, 2024

Conversation

niccokunzmann
Copy link
Member

@niccokunzmann niccokunzmann commented Oct 15, 2024

This is a contribution to #496.

@tobixen, could you help me understand this a bit better:
What I do not know about VTODO is commented here, too.
I am unclear about duration computation, too if DUE only or START only is set, it seems there is no duration, then.
In this way, this is very different from an event.
Could we talk about edge cases and such? I do not use VTODO but would like to have that implemented and known so that I can work on #716 and niccokunzmann/python-recurring-ical-events#186.

  • no properties
  • DTSTART only
  • DUE only
  • COMPLETED only
  • DUE vs. COMPLETED for duration or both
  • DURATION vs. DUE in case of COMPLETED
  • any other attributes that mark when a TODO starts or ends

Your feedback would be much appreciated.

002aa22 treats VTODO like VEVENT but it seems to me, there are slight differences.

@niccokunzmann niccokunzmann changed the title VTODO - start, end, duration [WIP] VTODO - start, end, duration Oct 15, 2024
@tobixen
Copy link
Contributor

tobixen commented Oct 15, 2024

I am unclear about duration computation, too if DUE only or START only is set, it seems there is no duration, then. In this way, this is very different from an event. Could we talk about edge cases and such? I do not use VTODO but would like to have that implemented and known so that I can work on #716 and niccokunzmann/python-recurring-ical-events#186.

  • no properties

If no properties are set, then all three (DTSTART, DUE, DURATION) are undefined. Setting one of the attributes should cause this attribute to be set in the returned object.

  • DTSTART only

DURATION is undefined. If DURATION is set, that should be considered equivalent to setting a DUE-date. It's probably appropriate to return an object with DTSTART and DURATION set in that case.

  • DUE only

DURATION is undefined. If DURATION is set, the object has to be populated with a DTSTART. Should the object be returned with a DUE or a DURATION? Those are mutually exclusive, so only one of the attributes should be returned. I'm in favor of keeping DUE.

  • COMPLETED only

DURATION is undefined. If DURATION is set, then populate the object with DURATION ... and I'd say, consider the DURATION to be time tracking data, and populate it with a DTSTART as well.

  • DUE vs. COMPLETED for duration or both
  • DURATION vs. DUE in case of COMPLETED

I've considered that DURATION should be used for the time estimate and not time tracking, from that perspective, ignore the COMPLETED for an object with both DUE and COMPLETED set. And also, since DUE and DURATION is mutually exclusive ... IMO that gives a strong hint that the intention is that one should consider DUE to be equivalent with DTSTART+DURATION.

  • any other attributes that mark when a TODO starts or ends

There aren't - at least not as I'm aware of. (perhaps some new RFCs have been posted since last I was studying this though).

@tobixen
Copy link
Contributor

tobixen commented Oct 15, 2024

If DURATION is defined in the object and one sets a new DUE, then I'd say the DTSTART should be moved rather than changing the DURATION.

If DTSTART and DUE is defined, and one attempts setting DURATION, then I also think DTSTART also should be moved.

This fits in with my use-cases ... though perhaps it's just my use-cases, I don't have strong opinions that it has to be this way. As said, I've chosen to define the DURATION as the time estimate, to consider DUE to be either a hard due or wishful thinking on when the task should be completed. Assuming those writing the standard considered DUE and DTSTART+DURATION to be equivalent, DTSTART is then efficiently the last possible moment one can start working on the task and theoretically complete it in time - assuming that the person working on the task does not need to sleep etc.

With my definitions, DUE may be important, DURATION may be a well thought-through variable, while the DTSTART is a value that does not have a strong real-world meaning (One ought to start with the task earlier than the defined DTSTART the way I consider those properties to be defined).

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Minor docstring wording change. Otherwise LGTM.

src/icalendar/tests/test_issue_716_alarm_extension.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 17, 2024

Pull Request Test Coverage Report for Build 11383520223

Details

  • 207 of 209 (99.04%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 97.528%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/icalendar/cal.py 65 67 97.01%
Totals Coverage Status
Change from base Build 11369442799: 0.04%
Covered Lines: 3602
Relevant Lines: 3690

💛 - Coveralls

Copy link
Member

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Minor grammar fixes.

src/icalendar/cal.py Outdated Show resolved Hide resolved
src/icalendar/cal.py Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@niccokunzmann
Copy link
Member Author

@tobixen Could you give this a review?

Here, I basically use the same algorithms in Event start & end for Todo start & end.

Altough they are different, I wonder how to deal with the differences.

There are two things to notice:

  • DTSTART, DUE and DURATION are the properties, allowing invalid states
  • start, end and duration are the calculated values with more verification, making sure the component is complete and not contradictory when calculated

I went through your requirements again.

Edge case to consider: For VTODO, DURATION seems to be ok to be set by itself without DTSTART. Is that true? In that case, we are not correct here for the Todo.duration attribute.

niccokunzmann and others added 3 commits October 17, 2024 11:54
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
Co-authored-by: Steve Piercy <[email protected]>
@niccokunzmann
Copy link
Member Author

@stevepiercy, thanks for the review. I applied the changes :)

@niccokunzmann
Copy link
Member Author

@niccokunzmann
Copy link
Member Author

@stevepiercy The documentation looks better and better: ❤️ https://icalendar--733.org.readthedocs.build/en/733/api.html#icalendar.cal.Event

@niccokunzmann
Copy link
Member Author

In the end, and if it makes sense, I would like to have start, end and duration properties on all components. Missing to evaluate:

  • FreeBusy

@niccokunzmann niccokunzmann changed the title [WIP] VTODO - start, end, duration VTODO - start, end, duration Oct 17, 2024
@niccokunzmann
Copy link
Member Author

@tobixen I will merge it for now. Please use it one day and create a PR in case you notice how to improve it.
@stevepiercy Thanks for your review. I added your suggestions.

Over a week has passed and I would like this functionality for #737.

@niccokunzmann niccokunzmann merged commit 65e5e07 into collective:main Oct 26, 2024
18 checks passed
@niccokunzmann niccokunzmann deleted the issue-662-todo branch October 26, 2024 09:19
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.

4 participants