Skip to content
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

Extract a symlink's target type if possible, so it can have proper colors. #563

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsully
Copy link
Contributor

@dsully dsully commented Oct 8, 2021

Extract a symlink's target type if possible, so it can have proper colors.

Before:
CleanShot 2021-10-08 at 11 12 14@2x

After:

CleanShot 2021-10-08 at 11 11 43@2x


  • Use cargo fmt
  • Add necessary tests
  • Add changelog entry

@dsully dsully requested review from meain and Peltoche as code owners October 8, 2021 18:18
@codecov-commenter
Copy link

Codecov Report

Merging #563 (ea64953) into master (74c1206) will decrease coverage by 0.35%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
- Coverage   86.73%   86.38%   -0.36%     
==========================================
  Files          37       37              
  Lines        3800     3819      +19     
==========================================
+ Hits         3296     3299       +3     
- Misses        504      520      +16     
Impacted Files Coverage Δ
src/meta/symlink.rs 55.84% <20.00%> (-13.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74c1206...ea64953. Read the comment docs.

@zwpaper
Copy link
Member

zwpaper commented Oct 9, 2021

oh, my bad, I got you wrong! could you add some tests for this feature and we can pass the CI

@dsully
Copy link
Contributor Author

dsully commented Oct 9, 2021

Hey, what sort of tests would you like? Since it's pretty much just pattern matching.

@@ -120,6 +151,7 @@ mod tests {
fn test_symlink_render_default_invalid_target_withcolor() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like the test here, it checks the invalid target color, IMO, you can add some tests like this testing some of newly added cases

Copy link
Member

@zwpaper zwpaper Oct 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe It looks a little dumb by adding tests for pattern matching, but this is how we kept the functionalities working as expected during the code changes.

@meain
Copy link
Member

meain commented Oct 9, 2021

We should probably colors the items using lscolors and not just builtin colors.

@zwpaper
Copy link
Member

zwpaper commented Oct 10, 2021

@meain the coloring is using the default mechanism, it would already use the LSCOLOR if I get the coloring logic right.

@meain
Copy link
Member

meain commented Oct 10, 2021

Ahh, my bad I misread what the code was doing.

@dsully I found a small issue in the code. If the symlink is relative and we are not calling lsd from the same directory that it was symlinked from, it does not work.

cd /tmp
mkdir -p test/{a,b}
cd test
touch a/file
cd b
ln -s ../a/file file
cd ..
lsd -ld b/file

@dsully
Copy link
Contributor Author

dsully commented Oct 12, 2021

Ok, I'll take a look.

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.

4 participants