Skip to content

Commit 0f7ef21

Browse files
chore(path): make path reading api more rusty (#9103)
### Description Remove `read_existing_to_string_or` in favor of `read_existing_to_string` to consolidate on the more Rust-y API for reading a path to a file. `read_existing_to_string_or` is an awkward API due: - It conflates two outcomes: The file not existing and the file existing with the contents of the default - The default is provided as `Result<impl Into<String>, io::Error>`. `impl Into<String>` was chosen so unnecessary work could be avoided. This is limiting compared to `FnOnce() -> String`. - It prevents us from using the battle tested `Option` API The benefits of not conflating the two outcomes is best displayed when trying to read a config file. Instead of defaulting having a default of `{}` and then parsing it as JSON we can skip the parse and just use `ConfigurationOptions::default()` ### Testing Instructions Existing unit tests + 👀
1 parent ac57dc7 commit 0f7ef21

File tree

5 files changed

+29
-37
lines changed

5 files changed

+29
-37
lines changed

crates/turborepo-lib/src/commands/link.rs

+9-6
Original file line numberDiff line numberDiff line change
@@ -215,11 +215,12 @@ pub async fn link(
215215

216216
let local_config_path = base.local_config_path();
217217
let before = local_config_path
218-
.read_existing_to_string_or(Ok("{}"))
218+
.read_existing_to_string()
219219
.map_err(|e| config::Error::FailedToReadConfig {
220220
config_path: local_config_path.clone(),
221221
error: e,
222-
})?;
222+
})?
223+
.unwrap_or_else(|| String::from("{}"));
223224

224225
let no_preexisting_id = unset_path(&before, &["teamid"], false)?.unwrap_or(before);
225226
let no_preexisting_slug =
@@ -314,11 +315,12 @@ pub async fn link(
314315

315316
let local_config_path = base.local_config_path();
316317
let before = local_config_path
317-
.read_existing_to_string_or(Ok("{}"))
318+
.read_existing_to_string()
318319
.map_err(|error| config::Error::FailedToReadConfig {
319320
config_path: local_config_path.clone(),
320321
error,
321-
})?;
322+
})?
323+
.unwrap_or_else(|| String::from("{}"));
322324

323325
let no_preexisting_id = unset_path(&before, &["teamid"], false)?.unwrap_or(before);
324326
let no_preexisting_slug =
@@ -541,11 +543,12 @@ fn add_turbo_to_gitignore(base: &CommandBase) -> Result<(), io::Error> {
541543
fn add_space_id_to_turbo_json(base: &CommandBase, space_id: &str) -> Result<(), Error> {
542544
let turbo_json_path = base.repo_root.join_component("turbo.json");
543545
let turbo_json = turbo_json_path
544-
.read_existing_to_string_or(Ok("{}"))
546+
.read_existing_to_string()
545547
.map_err(|error| config::Error::FailedToReadConfig {
546548
config_path: turbo_json_path.clone(),
547549
error,
548-
})?;
550+
})?
551+
.unwrap_or_else(|| String::from("{}"));
549552

550553
let space_id_json_value = format!("\"{}\"", space_id);
551554

crates/turborepo-lib/src/commands/login.rs

+6-4
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,12 @@ pub async fn sso_login(
3737

3838
let global_config_path = base.global_config_path()?;
3939
let before = global_config_path
40-
.read_existing_to_string_or(Ok("{}"))
40+
.read_existing_to_string()
4141
.map_err(|e| config::Error::FailedToReadConfig {
4242
config_path: global_config_path.clone(),
4343
error: e,
44-
})?;
44+
})?
45+
.unwrap_or_else(|| String::from("{}"));
4546

4647
let after = set_path(&before, &["token"], &format!("\"{}\"", token.into_inner()))?;
4748

@@ -92,11 +93,12 @@ pub async fn login(
9293

9394
let global_config_path = base.global_config_path()?;
9495
let before = global_config_path
95-
.read_existing_to_string_or(Ok("{}"))
96+
.read_existing_to_string()
9697
.map_err(|e| config::Error::FailedToReadConfig {
9798
config_path: global_config_path.clone(),
9899
error: e,
99-
})?;
100+
})?
101+
.unwrap_or_else(|| String::from("{}"));
100102
let after = set_path(&before, &["token"], &format!("\"{}\"", token.into_inner()))?;
101103

102104
global_config_path

crates/turborepo-lib/src/commands/unlink.rs

+6-4
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,12 @@ fn unlink_remote_caching(base: &mut CommandBase) -> Result<(), cli::Error> {
2323
let local_config_path = base.local_config_path();
2424

2525
let before = local_config_path
26-
.read_existing_to_string_or(Ok("{}"))
26+
.read_existing_to_string()
2727
.map_err(|error| config::Error::FailedToReadConfig {
2828
config_path: local_config_path.clone(),
2929
error,
30-
})?;
30+
})?
31+
.unwrap_or_else(|| String::from("{}"));
3132
let no_id = unset_path(&before, &["teamid"], false)?.unwrap_or(before);
3233
let no_slug = unset_path(&no_id, &["teamslug"], false)?.unwrap_or(no_id);
3334

@@ -62,11 +63,12 @@ fn unlink_spaces(base: &mut CommandBase) -> Result<(), cli::Error> {
6263
if needs_disabling {
6364
let local_config_path = base.local_config_path();
6465
let before = local_config_path
65-
.read_existing_to_string_or(Ok("{}"))
66+
.read_existing_to_string()
6667
.map_err(|e| config::Error::FailedToReadConfig {
6768
config_path: local_config_path.clone(),
6869
error: e,
69-
})?;
70+
})?
71+
.unwrap_or_else(|| String::from("{}"));
7072
let no_id = unset_path(&before, &["teamid"], false)?.unwrap_or(before);
7173
let no_slug = unset_path(&no_id, &["teamslug"], false)?.unwrap_or(no_id);
7274

crates/turborepo-lib/src/config/file.rs

+8-7
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,18 @@ impl ResolvedConfigurationOptions for ConfigFile {
2525
&self,
2626
_existing_config: &ConfigurationOptions,
2727
) -> Result<ConfigurationOptions, Error> {
28-
let mut contents = self
28+
let contents = self
2929
.path
30-
.read_existing_to_string_or(Ok("{}"))
30+
.read_existing_to_string()
3131
.map_err(|error| Error::FailedToReadConfig {
3232
config_path: self.path.clone(),
3333
error,
34-
})?;
35-
if contents.is_empty() {
36-
contents = String::from("{}");
37-
}
38-
let global_config: ConfigurationOptions = serde_json::from_str(&contents)?;
34+
})?
35+
.filter(|s| !s.is_empty());
36+
37+
let global_config = contents
38+
.as_deref()
39+
.map_or_else(|| Ok(ConfigurationOptions::default()), serde_json::from_str)?;
3940
Ok(global_config)
4041
}
4142
}

crates/turborepo-paths/src/absolute_system_path.rs

-16
Original file line numberDiff line numberDiff line change
@@ -441,22 +441,6 @@ impl AbsoluteSystemPath {
441441
fs::read_to_string(&self.0)
442442
}
443443

444-
/// Attempts to read a file, and:
445-
/// If the file does not exist it returns the default value.
446-
/// For all other scenarios passes through the `read_to_string` results.
447-
pub fn read_existing_to_string_or<I>(
448-
&self,
449-
default_value: Result<I, io::Error>,
450-
) -> Result<String, io::Error>
451-
where
452-
I: Into<String>,
453-
{
454-
match self.read_existing_to_string()? {
455-
Some(contents) => Ok(contents),
456-
None => default_value.map(|value| value.into()),
457-
}
458-
}
459-
460444
/// Attempts to read a file returning None if the file does not exist
461445
/// For all other scenarios passes through the `read_to_string` results.
462446
pub fn read_existing_to_string(&self) -> Result<Option<String>, io::Error> {

0 commit comments

Comments
 (0)