From aa3b79ec638c1a05e19ca262ed8ab129ab38d41d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 30 Jan 2024 10:58:45 -0800 Subject: [PATCH] Prompt user for missing `-r` and `-e` flags in `pip install` (#1180) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary If the user runs a command like `pip install requirements.txt`, we now prompt them to ask if they meant to include the `-r` flag: ![Screenshot 2024-01-29 at 8 38 29 PM](https://github.com/astral-sh/puffin/assets/1309177/82b9f7a2-2526-4144-b200-a5015e5b8a4b) ![Screenshot 2024-01-29 at 8 38 33 PM](https://github.com/astral-sh/puffin/assets/1309177/bd8ebb51-2537-4540-a0e0-718e66a1c69c) The specific logic is: if the requirement ends in `.txt` or `.in`, and the file exists locally, prompt the user for `-r`. If the requirement contains a directory separator, and the directory exists locally, prompt the user for `-e`. Closes #1166. --- Cargo.lock | 1 + Cargo.toml | 1 + crates/puffin/Cargo.toml | 3 +- crates/puffin/src/commands/pip_compile.rs | 2 +- crates/puffin/src/confirm.rs | 52 +++++++++++++++++++++++ crates/puffin/src/main.rs | 29 ++++++++----- crates/puffin/src/requirements.rs | 52 +++++++++++++++++++++-- 7 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 crates/puffin/src/confirm.rs diff --git a/Cargo.lock b/Cargo.lock index 0d4a7012f..50a00af47 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2360,6 +2360,7 @@ dependencies = [ "assert_fs", "chrono", "clap", + "console", "distribution-filename", "distribution-types", "dunce", diff --git a/Cargo.toml b/Cargo.toml index b6505fa6e..19b014dc0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,7 @@ cargo-util = { version = "0.2.8" } chrono = { version = "0.4.31" } clap = { version = "4.4.13" } configparser = { version = "3.0.4" } +console = { version = "0.15.8", default-features = false } csv = { version = "1.3.0" } dashmap = { version = "5.5.3" } data-encoding = { version = "2.5.0" } diff --git a/crates/puffin/Cargo.toml b/crates/puffin/Cargo.toml index 10ec8aba1..bd23298a4 100644 --- a/crates/puffin/Cargo.toml +++ b/crates/puffin/Cargo.toml @@ -47,6 +47,8 @@ anstream = { workspace = true } anyhow = { workspace = true } chrono = { workspace = true } clap = { workspace = true, features = ["derive"] } +console = { workspace = true } +dunce = { workspace = true } fs-err = { workspace = true, features = ["tokio"] } futures = { workspace = true } indicatif = { workspace = true } @@ -68,7 +70,6 @@ tracing-tree = { workspace = true } url = { workspace = true } waitmap = { workspace = true } which = { workspace = true } -dunce = "1.0.4" [target.'cfg(target_os = "windows")'.dependencies] mimalloc = "0.1.39" diff --git a/crates/puffin/src/commands/pip_compile.rs b/crates/puffin/src/commands/pip_compile.rs index 69eddc02b..56a0bbb7b 100644 --- a/crates/puffin/src/commands/pip_compile.rs +++ b/crates/puffin/src/commands/pip_compile.rs @@ -118,7 +118,7 @@ pub(crate) async fn pip_compile( .filter(|_| !upgrade.is_all()) .filter(|output_file| output_file.exists()) .map(Path::to_path_buf) - .map(RequirementsSource::from) + .map(RequirementsSource::from_path) .as_ref() .map(|source| RequirementsSpecification::from_source(source, &extras)) .transpose()? diff --git a/crates/puffin/src/confirm.rs b/crates/puffin/src/confirm.rs new file mode 100644 index 000000000..ffb2c579c --- /dev/null +++ b/crates/puffin/src/confirm.rs @@ -0,0 +1,52 @@ +use anyhow::Result; +use console::{style, Key, Term}; + +/// Prompt the user for confirmation in the given [`Term`]. +/// +/// This is a slimmed-down version of [`dialoguer::Confirm`], with the post-confirmation report +/// enabled. +pub(crate) fn confirm(message: &str, term: &Term, default: bool) -> Result { + let prompt = format!( + "{} {} {} {} {}", + style("?".to_string()).for_stderr().yellow(), + style(message).for_stderr().white().bold(), + style("[y/n]").for_stderr().black().bright(), + style("›").for_stderr().black().bright(), + style(if default { "yes" } else { "no" }) + .for_stderr() + .cyan(), + ); + + term.write_str(&prompt)?; + term.hide_cursor()?; + term.flush()?; + + // Match continuously on every keystroke, and do not wait for user to hit the + // `Enter` key. + let response = loop { + let input = term.read_key()?; + match input { + Key::Char('y' | 'Y') => break true, + Key::Char('n' | 'N') => break false, + Key::Enter => break default, + _ => {} + }; + }; + + let report = format!( + "{} {} {} {}", + style("✔".to_string()).for_stderr().green(), + style(message).for_stderr().white().bold(), + style("·").for_stderr().black().bright(), + style(if response { "yes" } else { "no" }) + .for_stderr() + .cyan(), + ); + + term.clear_line()?; + term.write_line(&report)?; + term.show_cursor()?; + term.flush()?; + + Ok(response) +} diff --git a/crates/puffin/src/main.rs b/crates/puffin/src/main.rs index d95d45eec..ec56497c8 100644 --- a/crates/puffin/src/main.rs +++ b/crates/puffin/src/main.rs @@ -40,6 +40,7 @@ static GLOBAL: tikv_jemallocator::Jemalloc = tikv_jemallocator::Jemalloc; mod commands; mod compat; +mod confirm; mod logging; mod printer; mod requirements; @@ -670,17 +671,17 @@ async fn run() -> Result { let requirements = args .src_file .into_iter() - .map(RequirementsSource::from) + .map(RequirementsSource::from_path) .collect::>(); let constraints = args .constraint .into_iter() - .map(RequirementsSource::from) + .map(RequirementsSource::from_path) .collect::>(); let overrides = args .r#override .into_iter() - .map(RequirementsSource::from) + .map(RequirementsSource::from_path) .collect::>(); let index_urls = IndexLocations::from_args( args.index_url, @@ -739,7 +740,7 @@ async fn run() -> Result { let sources = args .src_file .into_iter() - .map(RequirementsSource::from) + .map(RequirementsSource::from_path) .collect::>(); let reinstall = Reinstall::from_args(args.reinstall, args.reinstall_package); let no_binary = NoBinary::from_args(args.no_binary, args.no_binary_package); @@ -768,19 +769,23 @@ async fn run() -> Result { let requirements = args .package .into_iter() - .map(RequirementsSource::Package) + .map(RequirementsSource::from_package) .chain(args.editable.into_iter().map(RequirementsSource::Editable)) - .chain(args.requirement.into_iter().map(RequirementsSource::from)) + .chain( + args.requirement + .into_iter() + .map(RequirementsSource::from_path), + ) .collect::>(); let constraints = args .constraint .into_iter() - .map(RequirementsSource::from) + .map(RequirementsSource::from_path) .collect::>(); let overrides = args .r#override .into_iter() - .map(RequirementsSource::from) + .map(RequirementsSource::from_path) .collect::>(); let index_urls = IndexLocations::from_args( args.index_url, @@ -827,9 +832,13 @@ async fn run() -> Result { let sources = args .package .into_iter() - .map(RequirementsSource::Package) + .map(RequirementsSource::from_package) .chain(args.editable.into_iter().map(RequirementsSource::Editable)) - .chain(args.requirement.into_iter().map(RequirementsSource::from)) + .chain( + args.requirement + .into_iter() + .map(RequirementsSource::from_path), + ) .collect::>(); commands::pip_uninstall(&sources, cache, printer).await } diff --git a/crates/puffin/src/requirements.rs b/crates/puffin/src/requirements.rs index c41d469ce..7f3e975e9 100644 --- a/crates/puffin/src/requirements.rs +++ b/crates/puffin/src/requirements.rs @@ -1,8 +1,9 @@ //! A standard interface for working with heterogeneous sources of requirements. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; +use console::Term; use fs_err as fs; use rustc_hash::FxHashSet; @@ -12,6 +13,8 @@ use puffin_fs::NormalizedDisplay; use puffin_normalize::{ExtraName, PackageName}; use requirements_txt::{EditableRequirement, FindLink, RequirementsTxt}; +use crate::confirm; + #[derive(Debug)] pub(crate) enum RequirementsSource { /// A package was provided on the command line (e.g., `pip install flask`). @@ -24,14 +27,57 @@ pub(crate) enum RequirementsSource { PyprojectToml(PathBuf), } -impl From for RequirementsSource { - fn from(path: PathBuf) -> Self { +impl RequirementsSource { + /// Parse a [`RequirementsSource`] from a [`PathBuf`]. + pub(crate) fn from_path(path: PathBuf) -> Self { if path.ends_with("pyproject.toml") { Self::PyprojectToml(path) } else { Self::RequirementsTxt(path) } } + + /// Parse a [`RequirementsSource`] from a user-provided string, assumed to be a package. + /// + /// If the user provided a value that appears to be a `requirements.txt` file or a local + /// directory, prompt them to correct it (if the terminal is interactive). + pub(crate) fn from_package(name: String) -> Self { + // If the user provided a `requirements.txt` file without `-r` (as in + // `puffin pip install requirements.txt`), prompt them to correct it. + #[allow(clippy::case_sensitive_file_extension_comparisons)] + if name.ends_with(".txt") || name.ends_with(".in") { + if Path::new(&name).is_file() { + let term = Term::stderr(); + if term.is_term() { + let prompt = format!( + "`{name}` looks like a requirements file but was passed as a package name. Did you mean `-r {name}`?" + ); + let confirmation = confirm::confirm(&prompt, &term, true).unwrap(); + if confirmation { + return Self::RequirementsTxt(name.into()); + } + } + } + } + + // If the user provided a path to a local directory without `-e` (as in + // `puffin pip install ../flask`), prompt them to correct it. + if name.contains('/') || name.contains('\\') { + if Path::new(&name).is_dir() { + let term = Term::stderr(); + if term.is_term() { + let prompt = + format!("`{name}` looks like a local directory but was passed as a package name. Did you mean `-e {name}`?"); + let confirmation = confirm::confirm(&prompt, &term, true).unwrap(); + if confirmation { + return Self::RequirementsTxt(name.into()); + } + } + } + } + + Self::Package(name) + } } #[derive(Debug, Default, Clone)]