emersion / go-imap

📥 An IMAP library for clients and servers
MIT License
2.07k stars 292 forks source link

POC: rewrite imapnum with generic #595

Closed qjebbs closed 6 months ago

qjebbs commented 7 months ago

This is a POC to rewrite the imapnum package with generics.

Benifits:

  1. no need to covert from/to numset with "unsafe" package

    (*imapnum.Set)(unsafe.Pointer(s))
  2. no need to code methods for each type if we don't override them
    // all methods are implemented in the generic type
    type SeqSet = imapnum.Set[uint32]
  3. no need to covert results of Nums() when override original:
    func (s UIDSet) Nums() ([]UID, bool) {
        nums, ok := s.numSet().Nums()
        return uidListFromNumList(nums), ok
    }

    with generic:

    func (s UIDSet) Nums() ([]UID, bool) {
        return imapnum.Set[UID](s).Nums()
    }
emersion commented 7 months ago

This approach doesn't seem great for end-user documentation.

qjebbs commented 7 months ago

This approach doesn't seem great for end-user documentation.

But I don't see any breaking changes to end users. It changes only the internal unexported underlying package imapnum.

emersion commented 7 months ago

On GoDoc, users will no longer see the various methods for UIDSet and SeqSet listed. They will just see the type alias, and will need to dig into the unexported package to find out what methods they can use and how they can iterate on the set.

qjebbs commented 7 months ago

Good point. We can implement SeqSet just like how UIDSet currently does instead of an alias.

emersion commented 7 months ago

That sounds better. The remaining downside is that the type definitions are more obscure than before now:

// before
type UIDSet []UIDRange

// after
type UIDSet numset.Set[UID]

So some internal types are leaked and it's less obvious that library users can for _, r := range set.

qjebbs commented 7 months ago

Yes, it's kind of give and take. You are the one who ultimately makes the decision.