Proposal for new TimeWindow API

[rinde]

Analysis

• The current API mixes mutators (&mut self) with functional style (new instance).
• Some names are confusing to what is changed and in what way
• There are inconsistencies in names
• There are missing functions (i.e. counterparts to existing functions)

New naming

• Use prepone/postpone to make unambiguously clear what is going to happen with start/end
• Consistent naming scheme that hopefully makes TimeWindow easy and predictable to use
• Always uses functional style. Always uses mutators (&mut self) in favor of functional style. In principle, it doesn't matter which style is used as long as it is consistent. However, some methods return a property that is calculated. Using the functional style for those methods would be awkward because the function would need to return a tuple. That's why mutators seem to be better in this case. Additionally, this leaves room to extend some functions with a return type in the future.

Naming changes overview

old signature	new signature	comment
duration(&self) -> Duration	length(&self) -> Duration	to distinguish the type (Duration) from the TimeWindow property (length)
with_new_start(self, start: Time) -> TimeWindow	with_start(&self, new_start: Time) -> Self
extend_start(&mut self, new_start: Time) -> Option<Duration>	prepone_start_to(&self, new_start: Time) -> Self	extend applies to the TW which made the old name hard to comprehend
extend_start_by(&self, duration: Duration) -> TimeWindow	prepone_start_by(&self, duration: Duration) -> Self	extend applies to the TW which made the old name hard to comprehend
<doesn't exist>	prepone_start_extend_to(&self, new_length: Duration) -> Self	Can be added in a follow-up 🎯
shrink_towards_end(&mut self, new_start: Time)	postpone_start_to(&self, new_start: Time) -> Self
<doesn't exist>	postpone_start_by(&self, duration: Duration) -> Self	Can be added in a follow-up 🎯
shrink_towards_end_to(self, new_duration: Duration) -> TimeWindow	postpone_start_shrink_to(&self, new_length: Duration) -> Self
with_new_end(self, end: Time) -> TimeWindow	with_end(&self, new_end: Time) -> Self
shrink_towards_start(&mut self, new_end: Time)	prepone_end_to(&self, new_end: Time) -> Self
<doesn't exist>	prepone_end_by(&self, duration: Duration) -> Self	Can be added in a follow-up 🎯
shrink_towards_start_to(self, new_duration: Duration) -> TimeWindow	prepone_end_shrink_to(&self, new_length: Duration) -> Self
extend_end(&mut self, new_end: Time) -> Option<Duration>	postpone_end_to(&self, new_end: Time) -> Self	extend applies to the TW which made the old name hard to comprehend
extend_end_by(&self, duration: Duration) -> TimeWindow	postpone_end_by(&self, duration: Duration) -> Self	extend applies to the TW which made the old name hard to comprehend
<doesn't exist>	postpone_end_extend_to(&self, new_length: Duration) -> Self	Can be added in a follow-up 🎯
from_duration(start: Time, duration: Duration) -> Self	from_length_starting_at(length: Duration, start: Time) -> Self
from_end(duration: Duration, end: Time) -> Self	from_length_ending_at(length: Duration, end: Time) -> Self

[Felerius]

Quick first impression (will read it more thoroughly later): huge 👍 on the idea, Chris and I had a bug just recently in the new convert.rs because of this confusing behavior. I would however intuitively slightly prefer using functional methods. There's only two such methods that return anything and I doubt that their return value is always used. You could always compute the return value yourself as the caller, or we could even go with a (Self, ...) tuple return type.

[urmaul]

I'm also in favor of functional style. TimeWindows are value objects, updating them essentially means creating new value. Functional style matches that better.

[rinde]

@Felerius I'll have a look into the usage of the return values. If we remove them then I also agree that functional is better.

[rinde]

@urmaul @Felerius Ok, I updated the code and table to a functional style since we didn't actually use the returned values anywhere.

[Felerius]

Small stylistic comments but overall really nice!

[fabian-braun]

I still spotted some weirdness for some of the constructors when negative durations are passed in and would suggest the following changes:

from_length_starting_at -> unsure.. maybe return Result::Err if input duration is negative? Or create a zero width TW at provided start time?
from_length_ending_at -> unsure.. maybe return Result::Err if input duration is negative? Or create a zero width TW at provided end time?
prepone_start_by  -> document&test that passing a negative duration is always a no-op (should be tested, because it relies on the behavior of with_end internally)
postpone_end_by   -> document&test that passing a negative duration is always a no-op

[rinde]

@fabian-braun Great point! I went for turning the negative durations to zero as that seems to be most in line with what we do in other functions.

[fabian-braun]

very nice!