-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python bindings: deprecate some usage of str / bytes #5988
Comments
There's a solid argument that lots of However there are cases where I am on the fence about whether we should be unfriendly to a fringe use case, in favor of preventing the huge majority of use cases from doing something wrong. |
when libtorrent loads a torrent, it sanitizes file paths/names as well as comment and creator strings. They should be valid UTF-8 strings (possibly with replacement characters for invalid sequences). Is this not your experience? Perhaps I'm a bit too ignorant of python, but I agree that How come |
Ahh, I didn't think about the fact that the underyling So I'm wrong; I guess there isn't a case where You're right, python
There's a particular danger of allowing For this reason and others, there has been steady momentum over the years to eliminate implicit encoding, or mixing Given all that, I think the burden is to come up with a good use case where mixing If I want to craft new torrents, I think Deluge uses a custom bencode function, not libtorrent's. Not sure why. |
Responding here from #6125
I think the tests for Test-wise, I think testing round-trip stability ( I do think round-trip stability is best, to improve clarity and reduce usage bugs. But unit tests should test guarantees, not design goals. I actually think we should add tests that |
I changed my mind about |
Currently, the python bindings generally allow
str
as input wherestd::string
orchar *
is expected internally. It attempts to convertstr
tobytes
with the default encoding.In some cases, I think this does more harm than good. I think we should deprecate this functionality (and fire
DeprecationWarning
).bencode(...)
should not allowstr
in its input, includingdict
keys: this causes unstable round-trips (v != bdecode(bencode(v))
) and I can't think of any benefit from ittorrent_info({"info": ...})
should not allowstr
in its input: as abovetorrent_handle.add_piece(i, "value")
should not acceptstr
: any use of this is likely unintentionalsha1_hash("value")
should not acceptstr
: any use of this is likely unintentionalsha1_hash.to_string()
: it's hard to imagine this was ever useful, outside unit testsgenerate_fingerprint()
should be deprecated, in favor of a version that returnsbytes
: I think it's sensible to acceptstr
here, but the return value is intended for use in binary protocols, and client code should handle it as suchsession.dht_get_mutable_item("key", "salt")
should not acceptstr
session.dht_put_mutable_item("private", "public", "data", "salt")
should not acceptstr
torrent_handle.rename_file(..., b"path")
should not acceptbytes
torrent_handle.set_ssl_certificate(..., passphrase="passphrase")
should not acceptstr
file_storage.add_file(b"path", ...)
should not acceptbytes
file_storage.add_file(..., linkpath=b"path")
should not acceptbytes
dht_mutable_item_alert.salt
isstr
, and should just be changed tobytes
. All other members of this alert are broken currently so I can't imagine it would marginally break anyone to just change the binding tobytes
dht_put_item_alert.salt
isstr
, and should bebytes
, as aboveI'm finished writing new unit tests and won't add any more items to this issue.
The text was updated successfully, but these errors were encountered: