Refinement of API Design with Introduction of AddTorrent2 Function #887
Conversation
`new` replaced with `isNew` in order to do not use a global name `new`
- It looks like current implementation - easy to migrate - It hide an internal details from library consumers - it allows to change this interface and implementations in any manner in future without breaking changes for consumers. - Fix naming: - File->Filename. User can use a real File object - Magnet -> MagnetURI. Like with file - user can use a url.Query or already parsed MagnetLink or so on
|
Thanks for looking into this. The issue mentions that AddTorrentOpt was the intended way forward. In particular there are 2 parts to the API: Things that must be set when a Torrent is added, and things that can be altered after adding. AddTorrentOpt was the first part of this, and the rest were to be made their own methods on Torrent. |
|
If you could reduce the diff by moving the methods to their original locations that would make reviewing the changes a lot easier. |
|
Sure i can move it back. The diff will be lilbit ugly, but probably more easier to understand that current one. |
|
So what's about AddTorrentOpt-should i change something right now? I am asking because in case with TorrentOpt struct you will have a god object that contains conflicted fields. For example InfoHash,Filename,MetaInfo and others. with this big struct we should check that only one of "source" fields are filled. Do i understand right? |
|
@anacrolix I suppose the new diff looks better. But I am not sure what you decide about the mechanism of torrent creation. If you expect any changes in PR - just let me know. |
|
I see what you're going for now. If by god object you mean "configuration" style structs, unfortunately they're a reality in Go that are easier when you embrace them. I'd refer you to examples like TLS.Config. They are grating, but they do actually work surprisingly well in the context of Go. I think that is where I was going with AddTorrentOpt (and unintentionally with TorrentSpec, which I just don't like it's too big and crosses between initialization and stuff that can be configured later. Your PR makes me think about the issue of having magnet links, torrent files, and infohashes and how having fields for each of those would make for a horrible config struct. To that end I think the API has a few parts: being able to unpack special representations (we have this for MetaInfo and Magnet). Adding torrents and magnet links to a Client (we have helpers for this, plus TorrentSpec which can be built from the earlier representations). Then the separation of things that must occur at initialization and things that can change at runtime. The last point is the interesting area: things that occur at initialization we have AddTorrentOpt, and I intentionally want to avoid having a config struct for runtime things. Go isn't good for that again: we can't easily snapshot the current runtime config, make changes and push it back in without all kinds of silliness. Maybe we could, but also people don't particularly like that style. How this maps back to the "do-all" intermediate configuration (currently TorrentSpec) is ugly. If you make changes to TorrentSpec, some of them can't modified at runtime, so it's kind of broken. |
|
Yes, I see you understand me. Right now you have all the options to implement AddTorrent (or similar function). But I have to say that it’s not entirely clear to me – did you make any decisions about the design or was it just thinking out loud? In any case, feel free to write some specific goals that I can achieve. Also, if it’s faster than describing the tasks - you can merge this PR and rewrite it yourself. I just need a safe and clear API as final user. PS: Probably it will be interested for you - I have an implementation for Dave Cheney Functional options https://github.com/kazhuravlev/options-gen. This was built for service/client level configuration, but also can be adopted to use in scenarios like the current one. |
|
Sorry there's no progress on this. There's a very long list of downstream users, and it's not a simple case of tearing down a lot of the deprecated API. The API has been very stable for 7 years or so, so I wouldn't worry about it changing on you. |
Related with #369
I have carefully reviewed the discussions in thread #243 regarding the design of a new API, but it appears that a definitive decision has not been reached. In response, I have proposed a straightforward solution that maintains simplicity and allows for future refactoring without introducing breaking changes.
Key Changes:
AddTorrent2to address the absence of an available name for the existingAddTorrentfunction. (do you have any idea about a new name?)AddTorrent2function.context.Contextwithin theAddTorrent2function to facilitate potential internal library use in the future.addTorrentReqinterface and its associated implementations to provide flexibility for future design modifications without causing disruptions for end-users.Important Note:
It has been observed that
AddTorrentInfoHashWithStoragebears a striking resemblance toAddTorrentOpt, with the only discernible difference being the absence oft.setInfoBytesLocked(opts.InfoBytes)in the new version. While I believe this modification is secure, it is crucial to bring attention to this point for thorough consideration.