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

Fixed --files argument to sort alphabetically #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Alexe1289
Copy link

@Alexe1289 Alexe1289 commented Jan 9, 2024

Now if --files argument is found it will sort by default alphabetically the name of the files. If --sort argument is also provided (for example tokei --files --sort blanks), the sorting for files will be ignored and it will only sort by given argument.
Before:
image

After:
image

Should fix XAMPPRocky#1053

Copy link

@alexandruradovici alexandruradovici left a comment

Choose a reason for hiding this comment

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

Did you run cargo fmt?

src/cli_utils.rs Outdated
@@ -337,14 +337,16 @@ impl<W: Write> Printer<W> {

if self.list_files {
self.print_subrow()?;

let mut reports: Vec<_> = language.reports.clone();

Choose a reason for hiding this comment

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

Why clone this?

Copy link
Author

@Alexe1289 Alexe1289 Jan 9, 2024

Choose a reason for hiding this comment

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

Because "language" is borrowed and sort_by function requires mutable access.

Copy link

Choose a reason for hiding this comment

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

You could create a new Vec<&R> from language.reports which stores the data type R and sort that one. You can use .map and .collect.

Copy link
Author

@Alexe1289 Alexe1289 Jan 9, 2024

Choose a reason for hiding this comment

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

I don't know if this is how I should do it, if I try like this to get a Vec , it says that the Copy Trait is not implemented.

image

And I can't use a Vec<&Report> because I would have to change the code below to accept &Report
let (a, b): (Vec<_>, Vec<_>) = reports .iter() .partition(|r| r.stats.blobs.is_empty());

Choose a reason for hiding this comment

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

Please use

post code here

to post code instead of pictures.

Choose a reason for hiding this comment

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

You want to get a Vec<&Report>, instead you are trying to take ownership here.

Copy link
Author

Choose a reason for hiding this comment

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

Pushed a commit now with the requested change. I will run fmt on it after feedback,

@Alexe1289
Copy link
Author

Alexe1289 commented Jan 9, 2024

Did you run cargo fmt?

No, I didn't.
How to run it? I get an error if I try :

image

@alexandruradovici
Copy link

Did you run cargo fmt?

No, I didn't. How to run it? I get an error if I try :

image

You might need to install the fmt rust component.

@alexandruradovici
Copy link

Looks good, please send it to upstream and place a link to it here.

@Alexe1289
Copy link
Author

Alexe1289 commented Jan 18, 2024

XAMPPRocky#1059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants