refactor: Changed visiblity of Torrent and related funcs#106
Merged
Conversation
kurealnum
added
low prio
Contributor
WalkthroughPublicly exposed the Torrent type and two of its accessors by changing their visibility from crate-private to public in crates/libtortillas/src/torrent/mod.rs. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/libtortillas/src/torrent/mod.rs (3)
57-66: Public info_dict looks good; consider an idiomatic simplification and brief docsThe borrowed return type avoids cloning and is a good choice. Minor clean-up:
- Use
as_ref().or_else(...)to simplify the control flow.- Add a short rustdoc comment since this is now part of the public API.
Apply this diff:
- pub fn info_dict(&self) -> Option<&Info> { - if let Some(info) = &self.info { - Some(info) - } else { - match &self.metainfo { - MetaInfo::Torrent(t) => Some(&t.info), - _ => None, - } - } - } + /// Returns a read-only view of the info dictionary if known. + /// + /// - For .torrent-based MetaInfo: always `Some(&Info)`. + /// - For magnet URIs: `Some(&Info)` only after `InfoBytes` have been received; otherwise `None`. + pub fn info_dict(&self) -> Option<&Info> { + self.info.as_ref().or_else(|| match &self.metainfo { + MetaInfo::Torrent(t) => Some(&t.info), + _ => None, + }) + }
68-79: Avoid panics in public API; also simplify the else branchNow that this method is public, the
expect(...)panics leak across the crate boundary. Consider returningOption<InfoHash>orResult<InfoHash, TorrentError>(or providing a non-panicking companion liketry_info_hash) to keep the library resilient.Also, you can lean on
MetaInfo::info_hash()in the else branch and remove the extra&inif let:Apply this diff:
- pub fn info_hash(&self) -> InfoHash { - if let Some(info) = &self.info_dict() { - info.hash().expect("Failed to compute info hash") - } else { - match &self.metainfo { - MetaInfo::Torrent(t) => t.info.hash().expect("Failed to compute info hash"), - MetaInfo::MagnetUri(m) => m - .info_hash() - .expect("Magnet URIs should always have info hashes"), - } - } - } + pub fn info_hash(&self) -> InfoHash { + if let Some(info) = self.info_dict() { + info.hash().expect("Failed to compute info hash") + } else { + self.metainfo + .info_hash() + .expect("Magnet URIs should always have info hashes") + } + }Optionally, add a non-panicking companion:
// Place anywhere within `impl Torrent` pub fn try_info_hash(&self) -> Option<InfoHash> { if let Some(info) = self.info_dict() { info.hash() } else { self.metainfo.info_hash() } }
186-213: Expose actor messages or wrap them in a public APITorrentRequest and TorrentResponse are still
pub(crate), so external crates cannot construct messages or callask(...)directly. Since bothPeerandTrackerare declaredpub, there’s no private-type-in-public-interface issue—however, the actor API remains inaccessible outside this crate.Consider one of the following approaches:
- Make
TorrentRequestandTorrentResponsepub, and ensure all their variant payload types (Bitfield,CurrentPeers,PeerCount,CurrentTrackers,InfoHash,HasInfoDict,Request) are public and reachable.- Keep the messages private and provide a public façade (e.g., a
TorrentClienthandle with typed async methods that wrapask(...)internally).File: crates/libtortillas/src/torrent/mod.rs (lines 186–213)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/libtortillas/src/torrent/mod.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/libtortillas/src/torrent/mod.rs (2)
crates/libtortillas/src/metainfo/file.rs (2)
Info(60-90)TorrentFile(17-32)crates/libtortillas/src/metainfo/mod.rs (1)
test_info_hash_with_torrent(77-88)
| }; | ||
|
|
||
| pub(crate) struct Torrent { | ||
| pub struct Torrent { |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Making Torrent public: ensure external reachability and intended external usability
- Public type with private fields is fine and non-breaking for internal invariants.
- If the intent is for external crates to use this actor, keep in mind that
TorrentRequest/TorrentResponse(Lines 188-213) are stillpub(crate), which makes the actor effectively crate-internal for messaging. Consider the API story here (see separate comment below). - Since new public surface was added, this warrants at least a minor version bump.
- Verify that all types appearing in now-public method signatures are publicly reachable from the crate root (e.g.,
metainfo::Info,hashes::InfoHash) to avoid “private type in public interface” errors.
Run this script to confirm the relevant modules/types are exported from the crate root(s):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching crate roots for public module or re-exports of torrent, hashes, and metainfo…"
fd -t f -a 'lib.rs' | while read -r f; do
echo
echo "== $f"
rg -nP '^\s*pub\s+mod\s+(torrent|hashes|metainfo)\b|^\s*pub\s+use\s+.*\b(torrent|hashes|metainfo)\b' "$f" || true
done
echo
echo "Checking public visibility of InfoHash and Info definitions…"
rg -nP '^\s*pub\s+struct\s+InfoHash\b|^\s*pub\s+struct\s+Info\b'Length of output: 849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for all occurrences of InfoHash..."
rg -n 'InfoHash' || true
echo
echo "Checking struct definition and visibility for InfoHash..."
rg -nP '^\s*(pub|pub\(.+\))\s+struct\s+InfoHash\b' -C2 || true
echo
echo "Locating TorrentRequest and TorrentResponse definitions with visibility..."
rg -nP '^\s*(pub|pub\(.+\))\s+struct\s+Torrent(Request|Response)\b' -C3 crates/libtortillas/src/torrentLength of output: 3877
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting metainfo module exports in metainfo/mod.rs…"
rg -nP 'mod\s+file\b|pub\s+use\s+file::Info\b' -C2 crates/libtortillas/src/metainfo/mod.rs || true
echo
echo "Locating TorrentRequest and TorrentResponse definitions in torrent/mod.rs…"
rg -nP '^(pub|pub\(.+\))?\s*(enum|struct|type)\s+Torrent(Request|Response)\b' -C5 crates/libtortillas/src/torrent/mod.rs || trueLength of output: 583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating TorrentRequest and TorrentResponse definitions and their visibility…"
rg -n 'enum TorrentRequest|enum TorrentResponse' -C3 crates/libtortillas/src/torrent/mod.rs || true
echo
echo "Filtering for pub or pub(crate) qualifiers on those definitions…"
rg -nP '^\s*(pub(?:\([^)]*\))?)\s+enum\s+Torrent(Request|Response)\b' -C2 crates/libtortillas/src/torrent/mod.rs || true
echo
echo "Detecting any public methods on Torrent that reference metainfo::Info…"
rg -nP '^\s*pub\s+fn\b.*\bInfo\b' -C1 crates/libtortillas/src/torrent/mod.rs || trueLength of output: 901
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for TorrentRequest definitions..."
rg -n 'TorrentRequest' -C3 crates/libtortillas/src || echo "TorrentRequest not found"
echo
echo "Searching for TorrentResponse definitions..."
rg -n 'TorrentResponse' -C3 crates/libtortillas/src || echo "TorrentResponse not found"Length of output: 15041
Public API inconsistencies: Info not exposed & messaging types still private
- The
torrent,hashes, andmetainfomodules are correctly exported insrc/lib.rs. hashes::InfoHashis publicly reachable (pub type InfoHash = Hash<20>insrc/hashes.rs).- Issue:
metainfo::Infolives in a privatefile.rsand is not re-exported, yet
pub fn info_dict(&self) -> Option<&Info>(incrates/libtortillas/src/torrent/mod.rs:56–58) returns it.
→ Fix by exposingInfo, e.g. incrates/libtortillas/src/metainfo/mod.rsadd:mod file; +pub use file::Info; mod magnet; - Messaging enums
TorrentRequest/TorrentResponse(lines 188–189 incrates/libtortillas/src/torrent/mod.rs) remainpub(crate), so external code cannot interact with theTorrentactor. If you intend external messaging, change these topub. - You’ve increased the crate’s public surface—bump at least the minor version.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/libtortillas/src/torrent/mod.rs around line 28 and referencing lines
~56–58 and ~188–189: the public API is inconsistent because Torrent::info_dict()
returns a type Info that is defined privately in metainfo/file.rs and the
messaging enums TorrentRequest/TorrentResponse are pub(crate) so external
callers cannot use the Torrent actor. Fix by re-exporting Info from the metainfo
module (add a pub use in crates/libtortillas/src/metainfo/mod.rs so Info becomes
part of the crate public API), change the visibility of TorrentRequest and
TorrentResponse from pub(crate) to pub if external messaging is intended, and
update the crate version (bump the minor) since the public surface has expanded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Single commit PR -- see #104
Summary by CodeRabbit