Skip to content

Commit d1403a4

Browse files
ilyazubilyazub
committed
Address PR review comments
- Remove duplicate mod declarations in main.rs; use serpapi_cli crate exports - Add pub mod commands to lib.rs - Fix Param::FromStr error type: &'static str → String (implements Error) - Validate empty API key early in login.rs before network calls - Seed cycle-detection set with initial params in search.rs - Fix path separator portability in integration test assertions - Restore HOME/XDG_CONFIG_HOME with remove_var when originally unset - Fix search_id extraction in e2e test using serde_json parsing - Fix test_locations ignore reason to reflect actual dependency - Run cargo test --all-targets in CI to include integration tests - Align README --max-pages description with actual behavior
1 parent e5ee830 commit d1403a4

File tree

9 files changed

+67
-47
lines changed

9 files changed

+67
-47
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ jobs:
4242
run: cargo clippy -- -D warnings
4343

4444
- name: Run unit tests
45-
run: cargo test --lib --bins
45+
run: cargo test --all-targets
4646

4747
e2e-test:
4848
name: E2E Test

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ serpapi login
118118
- `--jq <expr>` — Client-side jq filter applied to JSON output (same as `gh --jq`)
119119
- `--api-key <key>` — Override API key (takes priority over environment and config file)
120120
- `--all-pages` — Fetch all result pages and merge array fields across pages
121-
- `--max-pages <n>` — When used with `--all-pages`, limit fetching to the first `n` pages
121+
- `--max-pages <n>` — Limit fetching to the first `n` pages when paginating (can be used with or without `--all-pages`)
122122

123123
**⚠️ Important: Flag Position**
124124

‎src/commands/login.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,12 @@ pub async fn run() -> Result<(), CliError> {
1111
})?;
1212
let api_key = api_key.trim();
1313

14+
if api_key.is_empty() {
15+
return Err(CliError::UsageError {
16+
message: "API key cannot be empty.".to_string(),
17+
});
18+
}
19+
1420
let client = make_client(api_key)?;
1521
let result = client
1622
.account(HashMap::new())

‎src/commands/search.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ pub async fn run(
3737
let mut accumulated: Option<Value> = None;
3838
let mut seen: HashSet<String> = HashSet::new();
3939
let mut pages_fetched: usize = 0;
40+
seen.insert(canonical_params_key(&current_params));
4041

4142
loop {
4243
let result = tokio::time::timeout(

‎src/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
//! - [`params`] — Query parameter parsing (`key=value` syntax)
1111
//! - [`jq`] — Client-side jq filtering via the `jaq` library
1212
13+
pub mod commands;
1314
pub mod config;
1415
pub mod error;
1516
pub mod output;

‎src/main.rs‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,5 @@
11
use clap::Parser;
2-
3-
mod commands;
4-
mod output;
5-
mod error;
6-
mod config;
7-
mod params;
8-
mod jq;
2+
use serpapi_cli::{commands, output, error, config, params, jq};
93

104
#[derive(Parser, Debug)]
115
#[command(

‎src/params.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,20 @@ pub struct Param {
88
}
99

1010
impl FromStr for Param {
11-
type Err = &'static str;
11+
type Err = String;
1212

1313
fn from_str(s: &str) -> Result<Self, Self::Err> {
1414
let mut parts = s.splitn(2, '=');
1515
match (parts.next(), parts.next()) {
16-
(Some(""), Some(_)) => Err("Empty parameter key"),
16+
(Some(""), Some(_)) => Err("Empty parameter key".to_string()),
1717
(Some("api_key"), Some(_)) => {
18-
Err("Use --api-key or SERPAPI_KEY instead of passing api_key as a parameter")
18+
Err("Use --api-key or SERPAPI_KEY instead of passing api_key as a parameter".to_string())
1919
}
2020
(Some(key), Some(value)) => Ok(Self {
2121
key: key.to_string(),
2222
value: value.to_string(),
2323
}),
24-
(Some(""), None) | (None, _) => Err("Empty parameter"),
24+
(Some(""), None) | (None, _) => Err("Empty parameter".to_string()),
2525
(Some(value), None) => Ok(Self {
2626
key: "q".to_string(),
2727
value: value.to_string(),

‎tests/e2e.rs‎

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ fn test_account() {
7070
}
7171

7272
#[test]
73-
#[ignore = "requires live SERPAPI_KEY env var"]
73+
#[ignore = "requires live SerpAPI locations API (network access)"]
7474
fn test_locations() {
7575
cargo_bin_cmd!("serpapi")
7676
.arg("--json")
@@ -99,26 +99,25 @@ fn test_archive() {
9999

100100
if search_output.status.success() {
101101
let output_str = String::from_utf8_lossy(&search_output.stdout);
102-
if let Some(search_id_start) = output_str.find("search_id") {
103-
if let Some(quote_start) = output_str[search_id_start..].find('"') {
104-
let after_quote = search_id_start + quote_start + 1;
105-
if let Some(quote_end) = output_str[after_quote..].find('"') {
106-
let search_id = &output_str[after_quote..after_quote + quote_end];
107-
if !search_id.is_empty() && search_id != "search_id" {
108-
cargo_bin_cmd!("serpapi")
109-
.arg("--api-key")
110-
.arg(&api_key)
111-
.arg("--json")
112-
.arg("archive")
113-
.arg(search_id)
114-
.assert()
115-
.success()
116-
.stdout(
117-
predicate::str::contains("search_metadata")
118-
.or(predicate::str::contains("search_parameters")),
119-
);
120-
}
121-
}
102+
if let Ok(json) = serde_json::from_str::<serde_json::Value>(&output_str) {
103+
let search_id = json
104+
.get("search_metadata")
105+
.and_then(|m| m.get("id"))
106+
.and_then(|id| id.as_str())
107+
.unwrap_or("");
108+
if !search_id.is_empty() {
109+
cargo_bin_cmd!("serpapi")
110+
.arg("--api-key")
111+
.arg(&api_key)
112+
.arg("--json")
113+
.arg("archive")
114+
.arg(search_id)
115+
.assert()
116+
.success()
117+
.stdout(
118+
predicate::str::contains("search_metadata")
119+
.or(predicate::str::contains("search_parameters")),
120+
);
122121
}
123122
}
124123
}

‎tests/integration_tests.rs‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,10 @@ mod config_tests {
1111
#[test]
1212
fn test_config_path_ends_with_serpapi_config() {
1313
let path = config::config_path();
14-
let path_str = path.to_str().unwrap();
15-
assert!(
16-
path_str.ends_with("serpapi/config.toml"),
17-
"config path should end with serpapi/config.toml, got: {}",
18-
path_str
19-
);
14+
let file_name = path.file_name().and_then(|s| s.to_str()).unwrap_or("");
15+
assert_eq!(file_name, "config.toml", "config path should end with config.toml, got: {:?}", path);
16+
let parent_name = path.parent().and_then(|p| p.file_name()).and_then(|s| s.to_str()).unwrap_or("");
17+
assert_eq!(parent_name, "serpapi", "config path parent dir should be serpapi, got: {:?}", path);
2018
}
2119

2220
#[test]
@@ -60,28 +58,42 @@ mod config_tests {
6058

6159
let result = config::resolve_api_key(None);
6260

63-
if let Some(h) = orig_home { std::env::set_var("HOME", h); }
64-
if let Some(x) = orig_xdg { std::env::set_var("XDG_CONFIG_HOME", x); }
61+
match orig_home {
62+
Some(h) => std::env::set_var("HOME", h),
63+
None => std::env::remove_var("HOME"),
64+
}
65+
match orig_xdg {
66+
Some(x) => std::env::set_var("XDG_CONFIG_HOME", x),
67+
None => std::env::remove_var("XDG_CONFIG_HOME"),
68+
}
6569
std::fs::remove_dir_all(&tmp).ok();
6670

6771
assert_eq!(result.unwrap(), "file_key");
6872
}
6973

7074
#[test]
7175
fn test_resolve_api_key_missing_returns_error() {
72-
// Point HOME to a temp dir so load_api_key() finds no config file
7376
let _guard = HOME_MUTEX.lock().unwrap();
7477
let tmp = std::env::temp_dir().join("serpapi_test_no_config");
7578
std::fs::create_dir_all(&tmp).ok();
7679
let orig_home = std::env::var("HOME").ok();
80+
let orig_xdg = std::env::var("XDG_CONFIG_HOME").ok();
7781
std::env::set_var("HOME", &tmp);
82+
std::env::remove_var("XDG_CONFIG_HOME");
7883

7984
let result = config::resolve_api_key(None);
8085

8186
// Restore HOME
82-
if let Some(h) = orig_home {
83-
std::env::set_var("HOME", h);
87+
match orig_home {
88+
Some(h) => std::env::set_var("HOME", h),
89+
None => std::env::remove_var("HOME"),
90+
}
91+
// Restore XDG_CONFIG_HOME
92+
match orig_xdg {
93+
Some(x) => std::env::set_var("XDG_CONFIG_HOME", x),
94+
None => std::env::remove_var("XDG_CONFIG_HOME"),
8495
}
96+
8597
assert!(result.is_err());
8698
let err_msg = format!("{}", result.unwrap_err());
8799
assert!(err_msg.contains("No API key found"));
@@ -250,12 +262,19 @@ mod error_tests {
250262
let tmp = std::env::temp_dir().join("serpapi_test_no_key_usage");
251263
std::fs::create_dir_all(&tmp).ok();
252264
let orig_home = std::env::var("HOME").ok();
265+
let orig_xdg = std::env::var("XDG_CONFIG_HOME").ok();
253266
std::env::set_var("HOME", &tmp);
267+
std::env::remove_var("XDG_CONFIG_HOME");
254268

255269
let result = serpapi_cli::config::resolve_api_key(None);
256270

257-
if let Some(h) = orig_home {
258-
std::env::set_var("HOME", h);
271+
match orig_home {
272+
Some(h) => std::env::set_var("HOME", h),
273+
None => std::env::remove_var("HOME"),
274+
}
275+
match orig_xdg {
276+
Some(x) => std::env::set_var("XDG_CONFIG_HOME", x),
277+
None => std::env::remove_var("XDG_CONFIG_HOME"),
259278
}
260279
std::fs::remove_dir_all(&tmp).ok();
261280

0 commit comments

Comments
 (0)