Sort compact/detailed/markdown error output by file path#1622
Sort compact/detailed/markdown error output by file path#1622mre merged 5 commits intolycheeverse:masterfrom
Conversation
| fn sort_stat_map<T>(error_map: &HashMap<InputSource, HashSet<T>>) -> Vec<(&InputSource, &HashSet<T>)> | ||
| where T: Display | ||
| { | ||
| let mut errors: Vec<(&InputSource, &HashSet<T>)> = error_map.iter().collect(); | ||
|
|
||
| errors.sort_by(|(source, _), (other_source, _)| source.cmp(other_source)); | ||
|
|
||
| errors | ||
| } No newline at end of file |
There was a problem hiding this comment.
I think we should also sort the values, while we're at it:
fn sort_stat_map<T>(error_map: &HashMap<InputSource, HashSet<T>>) -> Vec<(&InputSource, Vec<&T>)>
where
T: Display + Ord,
{
let mut entries: Vec<_> = error_map
.iter()
.map(|(source, set)| {
let mut sorted_values: Vec<&T> = set.iter().collect();
sorted_values.sort();
(source, sorted_values)
})
.collect();
entries.sort_by_key(|(source, _)| *source);
entries
}Also, was there a reason why this isn't a method on ResponseStats?
https://github.com/lycheeverse/lychee/blob/master/lychee-bin/src/stats.rs#L53
I think it would be cool to say &stats.sorted_error_map() instead of super::sort_stat_map(&stats.error_map), but there might be a reason why it's not a method.
There was a problem hiding this comment.
Also, was there a reason why this isn't a method on ResponseStats?
I did originally plan to make it a method, but I ran into an issue in the markdown module. Specifically, I noticed that suggestions are iterated/formatted after the errors:
lychee/lychee-bin/src/formatters/stats/markdown.rs
Lines 103 to 112 in 3fb94e0
In order to keep the suggestion list consistent with the error list, I decided to make a function that could be used to sort the suggestion map as well.
I think this could also be done without sorting the suggestion map ( e.g. by looping through the sorted error keys and using them as suggestion keys ), but since it's my first PR I wanted to play it safe and keep code changes to a minimum. This is also why I opted to make a function that takes a &HashMap for input, so that the parameters for write_stats_per_input wouldn't need to be changed.
I think we should also sort the values, while we're at it:
Sounds good 👍 I can start adding this and working on the other remaining tasks after I get home from work.
There was a problem hiding this comment.
Yup, sounds good. If you like, feel free to add that as a comment to sort_stat_map. 👍
|
Awesome! Much appreciated. 😎 Here's what's left to do:
Let me know if anything is unclear, or you need help. |
- Add unit/integration tests for sort_stats_map - Add human-sort dependency for natural sorting
- Fix clippy warning - Make entry sorting case-insensitive in sort_stat_map
|
Unit/integration tests for I also added the human-sort crate to lychee-bin's dependencies so that the items can be sorted in natural order. human-sort doesn't have any additional dependencies. The |
|
@Sufud, thanks for your work. This was a great addition. |
Implements the feature requested in #1619
The helper function is named
sort_stat_mapas it's used for both errors and suggestions in the markdown module, and it could be used for otherResponseStatsmaps as well.sort_stat_mapreturns aVecof key-value pairs, it's only used for iterating through each pair so using a map isn't currently necessary.The JSON and raw formatters are unchanged since they don't loop through the
ResponseStatsmaps.This is my first pull request, so any feedback/suggestions are more than welcome and appreciated 👽