-
-
Notifications
You must be signed in to change notification settings - Fork 437
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
Add Version Sort #843
Add Version Sort #843
Conversation
It looks like I have some refactoring artifacts. I'll fix and add more documentation |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #843 +/- ##
==========================================
+ Coverage 86.52% 86.54% +0.02%
==========================================
Files 49 49
Lines 4840 4840
==========================================
+ Hits 4188 4189 +1
+ Misses 652 651 -1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/sort.rs
Outdated
use human_sort::compare; | ||
use crate::versionsort::compare; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you didn't meant to simply remove human-sort, but this is what you are doing here.
Did you forget to implement some cli / config switch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do mean to replace the sorting library. The new code is now its own separate crate and the PR now reflects that.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: juansc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I think it's cleaner to break out the sorting logic into a separate crate. Once it's published I'll update this PR to use that crate instead. The code in the create has been updated and has more robust testing |
hi @juansc, I have read the PR and vsort crate, and it seems great! please add a changelog here https://github.com/lsd-rs/lsd/blob/master/CHANGELOG.md and we should be good to go. /lgtm |
New changes are detected. LGTM label has been removed. |
@zwpaper Changelog has been added |
thanks so much @juansc |
Description
Addresses #801. The root cause is that the external library that is being used does not have sufficient test coverage and is no longer maintained.
I took a look at how GNU sort is implemented and it looked simple enough to be included into this repo. I added a few test cases, including the one from the issue.
I'm still pretty new to Rust, so any code/organizational feedback is greatly appreciated
TODO
cargo fmt