gdm85/go-libdeluge

Proposal: Convert to native go datatypes

Closed this issue · 3 comments

ailox commented

This library is acting as a way of interfacing go with deluge, and is doing a fantastic job.
I was thinking that it would be nice to automatically convert some of the values into something more usable, therefore I'd like to propose the following type changes for attributes of the TorrentStatus:

  • ActiveTime: int64 → time.Duration
  • AddedTime: float32 → time.Time
  • CompletedTime: int64 → time.Time
  • ETA: int64 → time.Duration
  • NextAnnounce: int64 → time.Duration
  • SeedingTime: int64 → time.Duration
  • State: string → enum

For Peer:

  • Seed: int64 → bool

and (optionally), since they don't really need to be int64:

  • NumSeeds: int64 → int
  • NumPeers: int64 → int
  • NumPieces: int64 → int
  • TotalPeers: int64 → int
  • TotalSeeds: int64 → int
  • File.Index: int64 → int

(PieceLength also falls in that category, but since it is given in bytes it is probably more idiomatic to keep it int64)

Since this is a greater breaking change, and it might (depending on implementation) require modifications to the gdm85/rencode project, I wanted to ask for your opinion about this first. Do you think it is worth it to abstract the API to be more "friendly", or should go-libdeluge stay a slim wrapper around the RPC methods?

gdm85 commented

This is related to what we discussed in #13; ideally I agree with you, in the sense that a Go package should be Go-idiomatic (as much as possible).

In this specific instance however we are interfacing with a Python server (deluge) and unfortunately its API was never meant to be used by non-python clients, thus we are "stuck" in the current situation: a translation layer between the Python-native data types and Go data types would inevitably be verbose (in code lines) and be a source of bugs (think about all the corner cases where this conversion falls short by loosing some detail and precision).

For these reasons I believe the current "ugly" data structures are more honest and easier to maintain; if one wants to pursue the goal you outlined, there are basically 3 approaches as I see it:

  1. implement all the glue code in go-libdeluge itself and expose only the better data structures
  2. add marshaller/unmarshaller semantics to go-rencode (I do not know what what modifications you had in mind for it, this is what I could think of) to directly marshal into such better data structures
  3. add getter methods in go-libdeluge to retrieve the nicer fields

Please mention any other approach you have thought of.

Keeping the go-libdeluge API consistently uniform to the deluge native API is currently of value and I would like to not breach that, thus at this stage (I do not believe the project is very mature) I am unwilling to pursue any of these 3, which all have pros/cons on their own merit.

ailox commented

After reading your thoughts, I think it makes sense to stick closely to the values actually returned by deluge. One possible solution to this would be a struct wrapping TorrentStatus, and providing the getters for easy handling. This way everyone could decide on their own whether they need the precision of the raw values, or the abstraction provided by the Wrapper.
This way no code changes would be required, and this could be implemented in a separate optional package.

This falls a little short for peers and files, but I think it would be a good compromise.

type AbstractedTorrentStatus struct {
    *TorrentStatus
}

func (ats AbstractedTorrentStatus) GetAddedTime() time.Time {
    sec, ns := math.Modf(ats.AddedTime)
    return time.Unix(int64(sec), int64(ns*1e9))
}
//…

torrent := AbstractedTorrentStatus{torrentStatus} // maybe constructor?
torrent.GetAddedTime()
gdm85 commented

You can do that already in another package (that is one of the strengths of Go).

This way everyone could decide on their own whether they need the precision of the raw values, or the abstraction provided by the Wrapper.

My concern is however about something more subtle than precision alone. Let's look at added_time in the returned Python maps:

  • the key might not be there at all (corresponds to nil in Go?)
  • value is -1 or 0 (it can be mapped into a time.Time object in Go however its representation can be confusing)
  • value is a positive integer (can be mapped to a time.Time object in Go)