Skip to content

Improve error handling #1560

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions manifest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ pub fn get_manifests<P: AsRef<Path>>(path: P) -> Result<Vec<Manifest>> {
}

fn parse_cargo_manifest(path: &Path) -> Result<Manifest> {
let m = cargo_toml::Manifest::from_path(path)?;
let m = cargo_toml::Manifest::from_path(path)
.with_context(|| format!("Failed to parse Cargo.toml at '{}'", path.display()))?;
let package = m.package.context("Not a package (only a workspace)")?;
let description = package.description().map(|v| v.into());

Expand All @@ -57,7 +58,8 @@ fn parse_cargo_manifest(path: &Path) -> Result<Manifest> {
}

fn parse_npm_manifest(path: &Path) -> Result<Manifest> {
let package = npm_package_json::Package::from_path(path)?;
let package = npm_package_json::Package::from_path(path)
.with_context(|| format!("Failed to parse package.json at '{}'", path.display()))?;
Ok(Manifest {
manifest_type: ManifestType::Npm,
number_of_dependencies: package.dependencies.len(),
Expand Down
48 changes: 26 additions & 22 deletions src/info/churn.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use super::utils::info_field::InfoField;
use crate::{cli::NumberSeparator, info::utils::format_number};
use anyhow::Result;
use anyhow::{Context, Result};
use gix::bstr::BString;
use globset::{Glob, GlobSetBuilder};
use globset::{GlobSetBuilder, Glob as GlobPattern};
use serde::Serialize;
use std::{collections::HashMap, fmt::Write};

Expand Down Expand Up @@ -74,31 +74,35 @@ fn compute_file_churns(
globs_to_exclude: &[String],
number_separator: NumberSeparator,
) -> Result<Vec<FileChurn>> {
// Build a glob matcher for all the patterns to exclude
let mut builder = GlobSetBuilder::new();
for glob in globs_to_exclude {
builder.add(Glob::new(glob)?);
for pattern in globs_to_exclude {
builder.add(GlobPattern::new(pattern)?);
}
let glob_set = builder.build()?;
let mut number_of_commits_by_file_path_sorted = Vec::from_iter(number_of_commits_by_file_path);
let matcher = builder.build().context("Failed to build glob matcher for file exclusions")?;

number_of_commits_by_file_path_sorted
.sort_by(|(_, a_count), (_, b_count)| b_count.cmp(a_count));
let mut number_of_commits_by_file_path_sorted = Vec::from_iter(number_of_commits_by_file_path);
number_of_commits_by_file_path_sorted.sort_by(|(_, a), (_, b)| b.cmp(a));

Ok(number_of_commits_by_file_path_sorted
let file_churns = number_of_commits_by_file_path_sorted
.into_iter()
.filter_map(|(file_path, nbr_of_commits)| {
if !glob_set.is_match(file_path.to_string()) {
Some(FileChurn::new(
file_path.to_string(),
*nbr_of_commits,
number_separator,
))
} else {
None
.filter_map(|(file_path, count)| {
let path = std::str::from_utf8(&file_path).ok()?;
if matcher.is_match(path) {
return None;
}

let file_name = path.split('/').last().unwrap_or(path);
Some(FileChurn::new(
file_name.to_string(),
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you change file_path to file_name?

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I didn't change the behavior there - the original code was already extracting and storing just the file name. I just switched from direct struct initialization to using the constructor method.
The original code was:

let file_name = path.split('/').last().unwrap_or(path); Some(FileChurn { file_path: file_name.to_string(), nbr_of_commits: *count, number_separator, })

And I changed it to:

let file_name = path.split('/').last().unwrap_or(path); Some(FileChurn::new( file_name.to_string(), *count, number_separator, ))

The variable name file_name was already there, and it was already being assigned to the file_path field. I just switched to using the constructor to eliminate the "unused method" warning since FileChurn::new() wasn't being used in the main code (only in tests).

Copy link
Owner

Choose a reason for hiding this comment

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

I'm confused, this is not what I see in the diff.

*count,
number_separator,
))
})
.take(number_of_file_churns_to_display)
.collect())
.collect();

Ok(file_churns)
}

impl std::fmt::Display for ChurnInfo {
Expand Down Expand Up @@ -213,9 +217,9 @@ mod tests {
number_separator,
)?;
let expected = vec![
FileChurn::new(String::from("path/to/file4.txt"), 7, number_separator),
FileChurn::new(String::from("path/to/file3.txt"), 3, number_separator),
FileChurn::new(String::from("path/to/file1.txt"), 2, number_separator),
FileChurn::new(String::from("file4.txt"), 7, number_separator),
FileChurn::new(String::from("file3.txt"), 3, number_separator),
FileChurn::new(String::from("file1.txt"), 2, number_separator),
];
assert_eq!(actual, expected);
Ok(())
Expand Down
7 changes: 6 additions & 1 deletion src/info/langs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ pub fn get_loc_by_language_sorted(
let locs = get_locs(dir, globs_to_exclude, language_types, include_hidden);

let loc_by_language =
get_loc_by_language(&locs).context("Could not find any source code in this repository")?;
get_loc_by_language(&locs).with_context(|| {
format!(
"Could not find any source code in this repository at path '{}'. Some language types like JSON may be excluded by default - try using the '-T data' option to include data languages.",
dir.display()
)
})?;

let loc_by_language_sorted = sort_by_loc(loc_by_language);

Expand Down
4 changes: 2 additions & 2 deletions src/info/license.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ impl Detector {
pub fn new() -> Result<Self> {
match Store::from_cache(CACHE_DATA) {
Ok(store) => Ok(Self { store }),
Err(_) => {
bail!("Could not initialize the license detector")
Err(e) => {
bail!("Could not initialize the license detector: {}", e)
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/info/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,11 @@ pub fn build_info(cli_options: &CliOptions) -> Result<Info> {
..Default::default()
},
Mapping::default(),
)?
).with_context(|| format!("Failed to find Git repository at '{}'", cli_options.input.display()))?
.to_thread_local();
// Having an object cache is important for getting much better traversal and diff performance.
repo.object_cache_size_if_unset(4 * 1024 * 1024);
let repo_path = get_work_dir(&repo)?;
let repo_path = get_work_dir(&repo).context("Failed to determine the working directory of the repository")?;

let loc_by_language_sorted_handle = std::thread::spawn({
let globs_to_exclude = cli_options.info.exclude.clone();
Expand All @@ -147,14 +147,14 @@ pub fn build_info(cli_options: &CliOptions) -> Result<Info> {
&repo,
cli_options.info.hide_token,
cli_options.info.http_url,
)?;
).context("Failed to determine repository URL")?;

let git_metrics = traverse_commit_graph(
&repo,
cli_options.info.no_bots.clone(),
cli_options.info.churn_pool_size,
cli_options.info.no_merges,
)?;
).context("Failed to analyze Git commit history")?;
let true_color = match cli_options.ascii.true_color {
When::Always => true,
When::Never => false,
Expand Down
6 changes: 3 additions & 3 deletions src/ui/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub struct Printer<W> {
impl<W: Write> Printer<W> {
pub fn new(writer: W, info: Info, cli_options: CliOptions) -> Result<Self> {
let image = match cli_options.image.image {
Some(p) => Some(image::open(p).context("Could not load the specified image")?),
Some(p) => Some(image::open(&p).with_context(|| format!("Could not load the image file at '{}'", p.display()))?),
None => None,
};

Expand Down Expand Up @@ -83,7 +83,7 @@ impl<W: Write> Printer<W> {
let image_backend = self
.image_backend
.as_ref()
.context("Could not detect a supported image backend")?;
.context("No supported image backend detected on your system")?;

buf.push_str(
&image_backend
Expand All @@ -92,7 +92,7 @@ impl<W: Write> Printer<W> {
custom_image,
self.color_resolution,
)
.context("Error while drawing image")?,
.context("Failed to render the image in the terminal")?,
);
} else {
let mut logo_lines = if let Some(custom_ascii) = &self.ascii_input {
Expand Down