Proposal: `(Period[T]) Periodic` and similar methods returns `iter.Seq[Time[T]]` instead of channel about synchro HOT 5 OPEN

Hello @nobishino !! Thank you for describing the proposal. I have indeed considered iterators in the past.

Regarding the proposal you have presented, honestly, I don't feel like the benefits outweigh the disruption to backward compatibility.

func (p Period[T]) Periodic(next func(Time[T]) Time[T]) periodical[T]

If there are benefits to implementing the iterator interface, perhaps we could provide an Iter() method similar to the Slice() method, so that we can enjoy the benefits you are proposing.

What do you think?

from synchro.

Thank you! I would add some more detail and options for my proposal.

First of all, my proposal is motivated not by specific use-case, but by my personal technical curiosity.
So feel free to decline for not having specific use-case 😸

honestly, I don't feel like the benefits outweigh the disruption to backward compatibility.

Thanks for feedback! Based on that, I came up with several options which keeps backward compatibility as follows.
Option 2, 2-2, and 3 are illustrated in my draft branch: https://github.com/nobishino/synchro/commits/wip-period-sequence-api

Option 1: Add no new API for iter.Seq

This is obvious and completely reasonable option 😃

pros:

• No API Addition.

cons:

• Not enjoying benefits which iter ecosystem would provide.

Option 2: Add Seq() method to periodical[T]

This corresponds to your Iter() method idea. (I wrote this as Seq accidentally, and don't have strong preference for either name.)

Sample implementation for this option is given here:
nobishino@f3e5495

Since iter package is not yet provided as standard library, I write the sample using my fake package github.com/nobishino/gocoro/iter.

pros:

• Simple to implement
• Add least new API (only 1 method to already existing type)

cons:

• When break statement is used, which is translated to false return value from yield function, the goroutine associated with periodical instance is not terminated. (illustrated in the next commit's test: nobishino@608b669)
  • And, I think users don't expect iter.Seq[Time] cause such resource leakage for break statement.

Option 2-2: Same as Option 2 but address goroutine termination

This is variant of Option 2, but addresses the goroutine termination issue.

If we keep backward compatibility, we would need somewhat awkward implementation like nobishino@608b669

pros:

• Add least new API (only 1 method to already existing type).
• No goroutine leakage.

cons:

• Implementation would be complex and awkward.

Option 3: Add new methods to Period[T] type which returns iter.Seq

Option 3 adds several methods to Period[T](not periodical[T]) which have same functionality to PeriodicXXX family but returns iter.Seq instead. This option needs following addition to Period[T]'s API:

• func (p Period[T]) Sequence(next func(Time[T]) Time[T) iter.Seq[Time[T]]
• func (p Period[T]) SequenceDuration(d time.Duration) iter.Seq[Time[T]]
• func (p Period[T]) SequenceDate(years int, months int, days int) iter.Seq[Time[T]]
• func (p Period[T]) SequenceAdvance(u1 Unit, u2 ...Unit) iter.Seq[Time[T]]
• func (p Period[T]) SequenceISODuration(duration string) iter.Seq[Time[T]]

Illustrated in nobishino@336c474

pros:

• Simple to implement.
• No goroutine leakage.

cons:

• 5 new public API needs to be added.

Summary

I have explained 4 options which keep backward compatibility. Comparing these, there seems several objectives we want to meet, but I couldn't find the way to meet all of these.

1. Conform to & enjoy benefits of iter ecosystem
2. Have least addition of public APIs
3. Have simple & readable implementation
4. Make sure no resource leakage is caused by natural usage

My personal preference is Option 3, which meets 1, 3, 4 but giving up 2.
If we think the benefit of 1 is not too huge, I would think Option 1 (not adding new feature at least for now) is good decision.

from synchro.

Thank you for the wonderful proposal of Pros and Cons, @nobishino. It has become clear to me how we can approach each of them.

The policy I support is as follows:

• I don't want to increase the number of APIs that are currently unlikely to be used.
  • The reason is that having similar APIs can lead to confusion among users about which one to use.
• I don't want to return types that depend on third-party packages.
  • This is because we would lose control if we depended solely on this package. There is a possibility of performing Breaking Changes when maintenance stops or when bugs occur that require fixes dependent on external factors. I want to avoid this as much as possible.

As for the direction, I think Option 2, 2-2 is better.

Although it was mentioned that it might become complicated, I believe that by implementing it in this way, where all processing for iteration is enclosed in the Seq() method, it won't become complex. What do you think?

func (p periodical[T]) Seq() iter.Seq[Time[T]] {
	return func(yield func(Time[T]) bool) {
		for current := range p {
			if !yield(current) {
				break
			}
		}
		go func() {
			for current := range p {
				// Prevents leakage of the sending goroutine by reading
				// all channels sent to periodical[T].
			}
		}()
	}
}

from synchro.

@Code-Hex Oh, that is very nice! I didn't come up with that receiver-side solution.

Now I am convinced that Option 2-2 (with your implementation) is good idea.

from synchro.

I would submit a PR when iter.Seq is released(or about to be released).

Here is WIP branch: nobishino@ce1f98f
I will continue from this WIP when iter.Seq is ready to use.
(By the way, that PR would need to update go.mod to require Go1.23.0)

from synchro.