Conversation
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces boolean append_slash with an IndicatorStyle enum (None, Slash, FileType, Classify); adds CLI flags (--file-type, -F/--classify, --no-indicators), config aliasing and precedence, refactors file-name indicator rendering and symlink display, reworks config deserialization, and updates tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client/CLI
participant Flags as Flags Parser (src/cli.rs)
participant Config as Config Loader (src/settings.rs)
participant Params as Params/Conversion (src/structs.rs)
participant Renderer as File Renderer (src/utils/file.rs)
CLI->>Flags: parse args (--file-type / -F / --no-indicators / --indicator-style)
CLI->>Config: read config (config.toml)
Flags->>Params: produce Flags.indicator_style (Option<IndicatorStyle>)
Config->>Params: produce RawParams -> Params (map append_slash -> indicator_style)
Params->>Renderer: merged Params (indicator_style resolved via Flags or Config)
Renderer->>Output: format names with indicator_suffix per IndicatorStyle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 34 |
| Duplication | -58 |
TIP This summary will be updated as you push new changes. Give us feedback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/integration.rs (1)
862-906: Consider adding symlink to non-Unix platforms gracefully.The tests at lines 862-906 create a symlink conditionally with
#[cfg(unix)], but then assert onlinkandlink@unconditionally. On non-Unix platforms, thelinkfile won't exist, potentially causing test failures or misleading passes.♻️ Suggested fix: guard the symlink assertions
assert!(stdout.contains("child/")); - assert!(stdout.contains("link")); - assert!(!stdout.contains("link@")); + #[cfg(unix)] + { + assert!(stdout.contains("link")); + assert!(!stdout.contains("link@")); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration.rs` around lines 862 - 906, In the two tests test_gnu_compat_mode_accepts_indicator_style_slash and test_gnu_compat_mode_accepts_short_p you create a symlink only under #[cfg(unix)] but then assert on "link" and "link@" unconditionally; update the tests to only perform the symlink-related assertions when the symlink exists (e.g., wrap those assertions in #[cfg(unix)] blocks or check std::fs::symlink_metadata(&link_path).is_ok() before asserting on stdout containing "link" and not containing "link@"), leaving the existing child/ assertion unchanged so non-Unix platforms skip the symlink checks and the tests remain deterministic.src/utils/file.rs (1)
428-445: The check order could cause executable FIFOs or sockets to be misclassified, but this is unlikely in practice.The function checks
is_executable()beforeis_fifo()andis_socket(). Sinceis_executable()only checks Unix permission bits without file type guards, a FIFO or socket with executable bits set would incorrectly return*instead of|or=. However, FIFOs and sockets should not have executable bits set in normal operation, making this a theoretical rather than practical concern. If stricter classification is desired, the checks could be reordered to verify FIFO and socket types before checking executable status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/file.rs` around lines 428 - 445, The function file_type_indicator_suffix currently checks is_executable(metadata) before testing metadata.file_type().is_fifo() and is_socket(), which can misclassify FIFO/socket files that happen to have executable bits set; update the check order so that directory, symlink, FIFO (metadata.file_type().is_fifo()), and socket (metadata.file_type().is_socket()) are tested before the classify_executables && is_executable(metadata) branch, preserving the classify_executables behavior and using the same return values ("/", "@", "|", "=", "*", "") to ensure FIFOs/sockets are classified correctly even if their permission bits include execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/usage.md`:
- Around line 15-16: Fix the unclosed backtick in the `-A` / `--almost-all`
description: locate the line mentioning `-A` / `--almost-all` (the string "`-A`
/ `--almost-all` - Show hidden files, but don't show `.` and `..") and add the
missing closing backtick around `--almost-all` so the markdown code span is
properly terminated (ensure the line reads something like `- `-A` /
`--almost-all` - Show hidden files, but don't show `.` and `..``).
In `@README.md`:
- Around line 101-102: Fix the unclosed inline code backtick in the `-A` /
`--almost-all` description: ensure the phrase "`--almost-all` - Show hidden
files, but don't show `.` and `..`" has matching opening and closing backticks
around `--almost-all` (and verify the surrounding list item formatting for `-p`
/ `--slash-dirs` remains unchanged); update the README entry for
`-A`/`--almost-all` so both code spans are properly closed.
In `@tests/crate/file.rs`:
- Around line 554-600: The two tests
test_create_file_info_omits_symlink_at_in_native_long_mode and
test_create_file_info_keeps_symlink_at_in_gnu_long_mode are duplicates: both
call create_file_info with long_format and assert that "link@ -> " is not
present; either rename the second test to reflect the actual behavior (e.g.,
test_create_file_info_omits_symlink_at_in_gnu_long_mode) or consolidate both
into a single parameterized/test-case that covers both modes, and update the
test name and assertions accordingly; reference create_file_info and the
indicator_suffix behavior when deciding which change to apply.
---
Nitpick comments:
In `@src/utils/file.rs`:
- Around line 428-445: The function file_type_indicator_suffix currently checks
is_executable(metadata) before testing metadata.file_type().is_fifo() and
is_socket(), which can misclassify FIFO/socket files that happen to have
executable bits set; update the check order so that directory, symlink, FIFO
(metadata.file_type().is_fifo()), and socket (metadata.file_type().is_socket())
are tested before the classify_executables && is_executable(metadata) branch,
preserving the classify_executables behavior and using the same return values
("/", "@", "|", "=", "*", "") to ensure FIFOs/sockets are classified correctly
even if their permission bits include execute.
In `@tests/integration.rs`:
- Around line 862-906: In the two tests
test_gnu_compat_mode_accepts_indicator_style_slash and
test_gnu_compat_mode_accepts_short_p you create a symlink only under
#[cfg(unix)] but then assert on "link" and "link@" unconditionally; update the
tests to only perform the symlink-related assertions when the symlink exists
(e.g., wrap those assertions in #[cfg(unix)] blocks or check
std::fs::symlink_metadata(&link_path).is_ok() before asserting on stdout
containing "link" and not containing "link@"), leaving the existing child/
assertion unchanged so non-Unix platforms skip the symlink checks and the tests
remain deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53c087b9-4bc7-41ec-abcf-32c7dc26cab1
📒 Files selected for processing (15)
README.mddocs/src/config.mddocs/src/usage.mdsrc/cli.rssrc/lib.rssrc/settings.rssrc/structs.rssrc/utils/file.rstests/crate/app.rstests/crate/cli.rstests/crate/file.rstests/crate/settings.rstests/integration.rstests/seams.rstests/structs.rs
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration.rs (1)
1018-1033: Consider adding a positive assertion for the symlink arrow.The test asserts
link ->is present but doesn't verify the target is shown. Adding an assertion likeassert!(stdout.contains("target.txt"))would strengthen the test by confirming the full symlink rendering in long mode.💡 Suggested enhancement
assert!(stdout.contains("link -> ")); assert!(!stdout.contains("link@ -> ")); + assert!(stdout.contains("target.txt"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration.rs` around lines 1018 - 1033, The test function test_gnu_compat_mode_omits_symlink_at_indicator_in_long_mode should also assert the symlink target is displayed: after running Command::cargo_bin("lsp") and capturing stdout via run_and_capture, add a positive assertion that stdout.contains("target.txt") (or the exact expected target name shown by the fixture created by create_indicator_fixture()) so the test validates the full symlink rendering in long mode in addition to checking for "link -> ".tests/crate/file.rs (1)
726-739: Consider adding an assertion for the@marker absence.For consistency with
test_format_symlink_display_name_short_format_omits_marker_without_indicator(line 569), this test could also assert that the@marker is absent when using the default indicator style.💡 Suggested enhancement
assert!(short.contains("link")); assert!(!short.contains('*')); + assert!(!short.contains('@'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/crate/file.rs` around lines 726 - 739, Update the test function test_format_symlink_display_name_unreadable_short_format_omits_marker_without_indicator to also assert that the generated short string does not contain the '@' marker; locate the call to format_symlink_display_name_with_dim("link", Path::new("/tmp/link"), Err(io::Error::other("boom")), &Params::default(), false) and add an assertion similar to assert!(!short.contains('@')) to mirror the existing consistency check from test_format_symlink_display_name_short_format_omits_marker_without_indicator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/crate/file.rs`:
- Around line 726-739: Update the test function
test_format_symlink_display_name_unreadable_short_format_omits_marker_without_indicator
to also assert that the generated short string does not contain the '@' marker;
locate the call to format_symlink_display_name_with_dim("link",
Path::new("/tmp/link"), Err(io::Error::other("boom")), &Params::default(),
false) and add an assertion similar to assert!(!short.contains('@')) to mirror
the existing consistency check from
test_format_symlink_display_name_short_format_omits_marker_without_indicator.
In `@tests/integration.rs`:
- Around line 1018-1033: The test function
test_gnu_compat_mode_omits_symlink_at_indicator_in_long_mode should also assert
the symlink target is displayed: after running Command::cargo_bin("lsp") and
capturing stdout via run_and_capture, add a positive assertion that
stdout.contains("target.txt") (or the exact expected target name shown by the
fixture created by create_indicator_fixture()) so the test validates the full
symlink rendering in long mode in addition to checking for "link -> ".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2c4f2b3-cfff-4360-9ca5-6b1beec8b268
📒 Files selected for processing (5)
README.mddocs/src/usage.mdsrc/utils/file.rstests/crate/file.rstests/integration.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/src/usage.md
- README.md
Signed-off-by: Grant Ramsay <seapagan@gmail.com>
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.
Summary
Implement configurable indicator styles across native and GNU compatibility modes.
What Changed
append_slashslash,file-type,classify, andnone@marker in long mode to match GNUlsSummary by CodeRabbit
New Features
Bug Fixes
Documentation