Skip to content

Conversation

@angrynode
Copy link
Contributor

I think this solves the problem in #553

If you think this PR is ok, i can add docs before merging.

@casey
Copy link
Owner

casey commented Nov 7, 2025

Thanks for the PR! Docs would be great.

One thing that's odd to me is the form_urlencoded crate. As far as I know, form encoding and URL encoding are at least slightly different and not interchangeable. Unless that's not the case, I would suggest using the urlencoding crate.

Don't worry about the clippy failures on CI, I'll get those in another PR.

@casey
Copy link
Owner

casey commented Nov 7, 2025

Lints fixed in #555, updated the branch.

@angrynode
Copy link
Contributor Author

I rebased on the latest commit instead of adding a merge commit (personal preference to keep a reduced git log, let me know if that's inappropriate).

I added some doc comments explaining the url-encoding. Let me know if i should add more docs somewhere.

I think form_urlencoded is the correct choice. I believe the magnet URI query is inspired by the HTTP URL queries which adhere to this standard. That's already what you're using in MagnetLink::parse when iterating over Url::query_pairs which uses that crate to decode the key/values. There may be subtleties i'm not aware of.

@casey casey enabled auto-merge (squash) November 8, 2025 06:55
@casey casey merged commit 5c39923 into casey:master Nov 8, 2025
8 checks passed
@casey
Copy link
Owner

casey commented Nov 8, 2025

I rebased on the latest commit instead of adding a merge commit (personal preference to keep a reduced git log, let me know if that's inappropriate).

Totally fine, I always squash before merging.

I removed the comments, I pretty much reserve comments for things which are super confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants