Skip to content

Commit

Permalink
chore: code review feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
peterhuene committed Nov 19, 2024
1 parent edd7acf commit f4e86bb
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 59 deletions.
2 changes: 1 addition & 1 deletion wdl-engine/src/stdlib/glob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::diagnostics::function_call_failed;
use crate::diagnostics::invalid_glob_pattern;

/// Returns the Bash expansion of the glob string relative to the task's
/// execution directory, and in the same order.
/// execution directory, and in the same order (i.e. lexicographical).
///
/// https://github.com/openwdl/wdl/blob/wdl-1.2/SPEC.md#glob
fn glob(context: CallContext<'_>) -> Result<Value, Diagnostic> {
Expand Down
55 changes: 36 additions & 19 deletions wdl-engine/src/stdlib/read_boolean.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
//! Implements the `read_boolean` function from the WDL standard library.
use std::fs;
use std::io::BufRead;
use std::io::BufReader;

use anyhow::Context;
use wdl_analysis::types::PrimitiveTypeKind;
use wdl_ast::Diagnostic;

Expand Down Expand Up @@ -30,25 +31,41 @@ fn read_boolean(context: CallContext<'_>) -> Result<Value, Diagnostic> {
.unwrap_file()
.as_str(),
);
let mut contents = fs::read_to_string(&path)
.with_context(|| format!("failed to read file `{path}`", path = path.display()))
.map_err(|e| function_call_failed("read_boolean", format!("{e:?}"), context.call_site))?;

contents.make_ascii_lowercase();
let read_error = |e: std::io::Error| {
function_call_failed(
"read_boolean",
format!("failed to read file `{path}`: {e}", path = path.display()),
context.call_site,
)
};

let invalid_contents = || {
function_call_failed(
"read_boolean",
format!(
"file `{path}` does not contain a boolean value on a single line",
path = path.display()
),
context.call_site,
)
};

let mut lines = BufReader::new(fs::File::open(&path).map_err(read_error)?).lines();
let mut line = lines
.next()
.ok_or_else(invalid_contents)?
.map_err(read_error)?;

if lines.next().is_some() {
return Err(invalid_contents());
}

Ok(contents
line.make_ascii_lowercase();
Ok(line
.trim()
.parse::<bool>()
.map_err(|_| {
function_call_failed(
"read_boolean",
format!(
"file `{path}` does not contain a single boolean value",
path = path.display()
),
context.call_site,
)
})?
.map_err(|_| invalid_contents())?
.into())
}

Expand All @@ -69,8 +86,8 @@ mod test {
fn read_boolean() {
let mut env = TestEnv::default();
env.write_file("foo", "true false hello world!");
env.write_file("bar", "\n\t\tTrUe \n");
env.write_file("baz", "\n\t\tfalse \n");
env.write_file("bar", "\t\tTrUe \n");
env.write_file("baz", "\t\tfalse \n");
env.insert_name("t", PrimitiveValue::new_file("bar"));
env.insert_name("f", PrimitiveValue::new_file("baz"));

Expand All @@ -86,7 +103,7 @@ mod test {
assert!(
diagnostic
.message()
.contains("does not contain a single boolean value")
.contains("does not contain a boolean value on a single line")
);

let value = eval_v1_expr(&mut env, V1::Two, "read_boolean('bar')").unwrap();
Expand Down
52 changes: 35 additions & 17 deletions wdl-engine/src/stdlib/read_float.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
//! Implements the `read_float` function from the WDL standard library.
use std::fs;
use std::io::BufRead;
use std::io::BufReader;

use anyhow::Context;
use wdl_analysis::types::PrimitiveTypeKind;
use wdl_ast::Diagnostic;

Expand All @@ -29,23 +30,40 @@ fn read_float(context: CallContext<'_>) -> Result<Value, Diagnostic> {
.unwrap_file()
.as_str(),
);
let contents = fs::read_to_string(&path)
.with_context(|| format!("failed to read file `{path}`", path = path.display()))
.map_err(|e| function_call_failed("read_float", format!("{e:?}"), context.call_site))?;

Ok(contents
let read_error = |e: std::io::Error| {
function_call_failed(
"read_float",
format!("failed to read file `{path}`: {e}", path = path.display()),
context.call_site,
)
};

let invalid_contents = || {
function_call_failed(
"read_float",
format!(
"file `{path}` does not contain a float value on a single line",
path = path.display()
),
context.call_site,
)
};

let mut lines = BufReader::new(fs::File::open(&path).map_err(read_error)?).lines();
let line = lines
.next()
.ok_or_else(invalid_contents)?
.map_err(read_error)?;

if lines.next().is_some() {
return Err(invalid_contents());
}

Ok(line
.trim()
.parse::<f64>()
.map_err(|_| {
function_call_failed(
"read_float",
format!(
"file `{path}` does not contain a single float value",
path = path.display()
),
context.call_site,
)
})?
.map_err(|_| invalid_contents())?
.into())
}

Expand All @@ -66,7 +84,7 @@ mod test {
fn read_float() {
let mut env = TestEnv::default();
env.write_file("foo", "12345.6789 hello world!");
env.write_file("bar", "\n\t\t12345.6789 \n");
env.write_file("bar", "\t \t 12345.6789 \n");
env.insert_name("file", PrimitiveValue::new_file("bar"));

let diagnostic =
Expand All @@ -81,7 +99,7 @@ mod test {
assert!(
diagnostic
.message()
.contains("does not contain a single float value")
.contains("does not contain a float value on a single line")
);

let value = eval_v1_expr(&mut env, V1::Two, "read_float('bar')").unwrap();
Expand Down
52 changes: 35 additions & 17 deletions wdl-engine/src/stdlib/read_int.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
//! Implements the `read_int` function from the WDL standard library.
use std::fs;
use std::io::BufRead;
use std::io::BufReader;

use anyhow::Context;
use wdl_analysis::types::PrimitiveTypeKind;
use wdl_ast::Diagnostic;

Expand All @@ -29,23 +30,40 @@ fn read_int(context: CallContext<'_>) -> Result<Value, Diagnostic> {
.unwrap_file()
.as_str(),
);
let contents = fs::read_to_string(&path)
.with_context(|| format!("failed to read file `{path}`", path = path.display()))
.map_err(|e| function_call_failed("read_int", format!("{e:?}"), context.call_site))?;

Ok(contents
let read_error = |e: std::io::Error| {
function_call_failed(
"read_int",
format!("failed to read file `{path}`: {e}", path = path.display()),
context.call_site,
)
};

let invalid_contents = || {
function_call_failed(
"read_int",
format!(
"file `{path}` does not contain an integer value on a single line",
path = path.display()
),
context.call_site,
)
};

let mut lines = BufReader::new(fs::File::open(&path).map_err(read_error)?).lines();
let line = lines
.next()
.ok_or_else(invalid_contents)?
.map_err(read_error)?;

if lines.next().is_some() {
return Err(invalid_contents());
}

Ok(line
.trim()
.parse::<i64>()
.map_err(|_| {
function_call_failed(
"read_int",
format!(
"file `{path}` does not contain a single integer value",
path = path.display()
),
context.call_site,
)
})?
.map_err(|_| invalid_contents())?
.into())
}

Expand All @@ -67,7 +85,7 @@ mod test {
fn read_int() {
let mut env = TestEnv::default();
env.write_file("foo", "12345 hello world!");
env.write_file("bar", "\n\t\t12345 \n");
env.write_file("bar", " \t \t12345 \n");
env.insert_name("file", PrimitiveValue::new_file("bar"));

let diagnostic = eval_v1_expr(&mut env, V1::Two, "read_int('does-not-exist')").unwrap_err();
Expand All @@ -81,7 +99,7 @@ mod test {
assert!(
diagnostic
.message()
.contains("does not contain a single integer value")
.contains("does not contain an integer value on a single line")
);

let value = eval_v1_expr(&mut env, V1::Two, "read_int('bar')").unwrap();
Expand Down
10 changes: 5 additions & 5 deletions wdl-engine/src/stdlib/read_json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ fn read_json(mut context: CallContext<'_>) -> Result<Value, Diagnostic> {
function_call_failed(
"read_json",
format!(
"failed to deserialize file `{path}`: {e}",
"failed to read JSON file `{path}`: {e}",
path = path.display()
),
context.call_site,
Expand Down Expand Up @@ -85,7 +85,7 @@ mod test {
assert!(
diagnostic
.message()
.contains("call to function `read_json` failed: failed to deserialize file")
.contains("call to function `read_json` failed: failed to read JSON file")
);
assert!(
diagnostic
Expand All @@ -97,7 +97,7 @@ mod test {
assert!(
diagnostic
.message()
.contains("call to function `read_json` failed: failed to deserialize file")
.contains("call to function `read_json` failed: failed to read JSON file")
);
assert!(
diagnostic
Expand Down Expand Up @@ -137,7 +137,7 @@ mod test {
assert!(
diagnostic
.message()
.contains("call to function `read_json` failed: failed to deserialize file")
.contains("call to function `read_json` failed: failed to read JSON file")
);
assert!(
diagnostic
Expand Down Expand Up @@ -167,7 +167,7 @@ mod test {
assert!(
diagnostic
.message()
.contains("call to function `read_json` failed: failed to deserialize file")
.contains("call to function `read_json` failed: failed to read JSON file")
);
assert!(
diagnostic
Expand Down

0 comments on commit f4e86bb

Please sign in to comment.