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

Fix Bug: Handle -L with broken symlink #457 #754

Merged
merged 9 commits into from Nov 27, 2022
Merged

Conversation

r3dArch
Copy link
Contributor

@r3dArch r3dArch commented Oct 14, 2022

This is a draft PR for #457.


TODO

  • Use cargo fmt
  • Add necessary tests
  • Add changelog entry
  • Update default config/theme in README (if applicable)
  • Update man page at lsd/doc/lsd.md (if applicable)

@r3dArch r3dArch changed the title Bug: Handle -L with broken symlink #457 Fix Bug: Handle -L with broken symlink #457 Oct 14, 2022
@meain
Copy link
Member

meain commented Nov 23, 2022

Sorry about the delayed response. I somehow missed the PR. This looks great. Could you add tests for this behavior and a changelog entry and we should be good to go.

@r3dArch
Copy link
Contributor Author

r3dArch commented Nov 24, 2022

Thank you for the response. I'll make those changes soon and let you know when they are ready.

@r3dArch
Copy link
Contributor Author

r3dArch commented Nov 25, 2022

@meain I have added tests and updated the changelog, although I do not have a Windows device to run the tests. According to the Rust docs, symlink creation on the Windows OS is a privileged action, so these tests will probably fail on Windows. Should I make these tests Unix only? What are your thoughts? Thanks.

@meain
Copy link
Member

meain commented Nov 26, 2022

@r3dArch The CI should be able to run Windows test. With that said, if the test are gonna fail on Windows, we can run it on just linux and macOS.

@meain
Copy link
Member

meain commented Nov 26, 2022

Merging another PR causing some conflicts, could you also take care of that :D.

@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2022

Codecov Report

Merging #754 (16606ff) into master (c48f0f4) will increase coverage by 0.13%.
The diff coverage is 78.51%.

@@            Coverage Diff             @@
##           master     #754      +/-   ##
==========================================
+ Coverage   86.60%   86.73%   +0.13%     
==========================================
  Files          44       44              
  Lines        4336     4434      +98     
==========================================
+ Hits         3755     3846      +91     
- Misses        581      588       +7     
Impacted Files Coverage Δ
src/display.rs 81.90% <55.88%> (-2.29%) ⬇️
src/meta/mod.rs 54.18% <78.00%> (+11.02%) ⬆️
src/meta/filetype.rs 80.55% <100.00%> (ø)
src/sort.rs 99.49% <100.00%> (+1.22%) ⬆️
tests/integration.rs 100.00% <100.00%> (ø)
src/meta/size.rs 98.18% <0.00%> (+1.21%) ⬆️
src/meta/symlink.rs 82.75% <0.00%> (+8.62%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@r3dArch
Copy link
Contributor Author

r3dArch commented Nov 26, 2022

@meain I fixed a clippy warning pointed out by the CI/CD and the conflicts caused by the other PR. Is there anything else needed for this to be merged? Clippy points out some other style problems, but they are not from the changes I made.

@meain meain merged commit f22ad5b into lsd-rs:master Nov 27, 2022
@meain
Copy link
Member

meain commented Nov 27, 2022

Thanks again for the PR :D.

@r3dArch
Copy link
Contributor Author

r3dArch commented Nov 27, 2022

Thank you. This was actually my first contribution to someone else's project, so it is really nice it got merged :D.

@meain
Copy link
Member

meain commented Nov 27, 2022

To more PRs! 🍷 🍾 😉

@r3dArch r3dArch deleted the bug-457 branch November 30, 2022 04:52
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.

None yet

3 participants