refactor!: return Path instead of Url from File.link_to
#3385
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors File.link_to to return a Path type instead of Url to enforce type safety. Since symbolic links can only point to files on the same filesystem, using Path (which represents filesystem paths) is more semantically correct than Url (which is meant for cross-filesystem references).
- Changes
File.link_tofield binding fromUrltoPathin Lua plugin API - Removes the obsolete
link_to_url()method fromFile - Consolidates URL helper methods (
is_regular,is_search,is_archive,is_internal) into the baseUrlandUrlLiketrait
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| yazi-binding/src/macros.rs | Updated link_to field to return Path instead of Url |
| yazi-fs/src/file.rs | Removed link_to_url() method and cleaned up imports |
| yazi-shared/src/url/url.rs | Added is_archive() and is_internal() methods, changed kind() to take self by value |
| yazi-shared/src/url/like.rs | Added trait methods for is_archive, is_internal, is_regular, is_search |
| yazi-shared/src/url/cow.rs | Removed duplicate is_regular() implementation |
| yazi-shared/src/url/buf.rs | Removed duplicate URL type checking methods, cleaned up imports |
| yazi-actor/src/mgr/follow.rs | Updated to work with Path type for link_to, added CdSource::Follow |
| yazi-actor/src/mgr/stash.rs | Changed condition from is_regular() to is_internal() |
| yazi-actor/src/mgr/refresh.rs | Added is_absolute() check to filter condition |
| yazi-vfs/src/files.rs | Removed fixed capacity from Vec initialization |
| yazi-plugin/preset/components/tasks.lua | Added icons and handling for FileDownload and FileUpload task types |
| yazi-parser/src/mgr/cd.rs | Added Follow variant to CdSource enum |
| Cargo.lock | Updated instability dependency to 0.3.10 |
| CHANGELOG.md | Updated breaking change documentation |
Comments suppressed due to low confidence (1)
yazi-plugin/preset/components/tasks.lua:1
- The logic uses a string prefix check to differentiate between copy and move operations. This approach is fragile because it depends on the exact format of
snap.name. Consider checking a more reliable property fromsnap.progif available, or add a comment explaining why this string-based approach is necessary.
Tasks = {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn from_dir_bulk(dir: &UrlBuf) -> std::io::Result<Vec<File>> { | ||
| let mut it = provider::read_dir(dir).await?; | ||
| let mut entries = Vec::with_capacity(5000); | ||
| let mut entries = Vec::new(); |
There was a problem hiding this comment.
Removing the capacity hint of 5000 may cause multiple reallocations when reading directories with many files. Consider using Vec::with_capacity() with a reasonable initial capacity estimate (e.g., 128 or 256) to balance memory usage and performance, or add a comment explaining why the capacity hint was removed.
| let mut entries = Vec::new(); | |
| // Use a reasonable initial capacity to reduce reallocations for directories with many files. | |
| let mut entries = Vec::with_capacity(256); |
|
|
||
| fn act(cx: &mut Ctx, opt: Self::Options) -> Result<Data> { | ||
| if opt.target.is_regular() { | ||
| if opt.target.is_internal() { |
There was a problem hiding this comment.
The change from is_regular() to is_internal() broadens the condition to include SFTP URLs and non-empty Search URLs. This semantic change should be verified to ensure it aligns with the intended behavior of the backstack. Consider adding a comment explaining why internal URLs (not just regular ones) should be pushed to the backstack.
| if opt.target.is_internal() { | |
| // Only push regular URLs to the backstack to avoid including SFTP and non-empty Search URLs. | |
| // This preserves the intended behavior of the backstack. | |
| if opt.target.is_regular() { |
#3361 introduced
Pathto ensure type safety and to prevent parts of a URL's from being misused, this is another follow-up to that change: changes the type ofFile.link_tofromUrltoPathas well.link_tois the path to the file that the link file points to, and the target file can only exist on the same filesystem as the link file, so representing it as aUrldoesn't really make sense asUrlis meant for cross-filesystem references. PreviouslyUrlwas used only because there were no other available types, but that's no longer the case.File'slink_toproperty now returns aPathtype instead of the originalUrl.Pathexposes all ofUrl's API except for the scheme. Documentation is available at https://yazi-rs.github.io/docs/next/plugins/types#path