lsd-rs / lsd

The next gen ls command
Apache License 2.0
13.19k stars 430 forks source link

Add Version Sort #843

Closed juansc closed 1 year ago

juansc commented 1 year ago

Description

Addresses https://github.com/lsd-rs/lsd/issues/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

juansc commented 1 year ago

It looks like I have some refactoring artifacts. I'll fix and add more documentation

codecov-commenter commented 1 year ago

Codecov Report

Merging #843 (858e437) into master (71156b8) will increase coverage by 0.02%. The diff coverage is n/a.

:exclamation: 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     
Impacted Files Coverage Δ
src/sort.rs 98.00% <ø> (ø)

... and 1 file with indirect coverage changes

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

muniu-bot[bot] commented 1 year ago

[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: - **[OWNERS](https://github.com/lsd-rs/lsd/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
juansc commented 1 year ago

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

zwpaper commented 1 year ago

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

muniu-bot[bot] commented 1 year ago

New changes are detected. LGTM label has been removed.

juansc commented 1 year ago

@zwpaper Changelog has been added

zwpaper commented 1 year ago

thanks so much @juansc