Skip to content

Commit

Permalink
refactor: enhance cli error messages for run and vet commands. (#589)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Peefy
Peefy committed Jun 29, 2023
1 parent a5ba758 commit cde5471
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 47 deletions.
8 changes: 5 additions & 3 deletions kclvm/config/src/settings.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Copyright 2021 The KCL Authors. All rights reserved.
use anyhow::Result;
use anyhow::{Context, Result};
use serde::{
de::{DeserializeSeed, Error, MapAccess, SeqAccess, Unexpected, Visitor},
Deserialize, Serialize,
Expand Down Expand Up @@ -344,8 +344,10 @@ pub struct TestSettingsFile {

/// Load kcl settings file.
pub fn load_file(filename: &str) -> Result<SettingsFile> {
let f = std::fs::File::open(filename)?;
let data: SettingsFile = serde_yaml::from_reader(f)?;
let f = std::fs::File::open(filename)
.with_context(|| format!("Failed to load '{}', no such file or directory", filename))?;
let data: SettingsFile = serde_yaml::from_reader(f)
.with_context(|| format!("Failed to load '{}', invalid setting file format", filename))?;
Ok(data)
}

Expand Down
2 changes: 1 addition & 1 deletion kclvm/runtime/src/value/val_plan.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ fn handle_schema(value: &ValueRef) -> (Vec<ValueRef>, bool) {

impl ValueRef {
fn is_planned_empty(&self) -> bool {
self.is_dict() && !self.is_truthy()
(self.is_dict() && !self.is_truthy()) || self.is_undefined()
}

pub fn plan_to_json_string(&self) -> String {
Expand Down
47 changes: 26 additions & 21 deletions kclvm/tools/src/vet/expr_builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ use kclvm_ast::{
use crate::util::loader::{DataLoader, Loader, LoaderKind};
use anyhow::{bail, Context, Result};

const FAIL_LOAD_VALIDATED_ERR_MSG: &str = "Failed to load the validated file";

trait ExprGenerator<T> {
fn generate(&self, value: &T, schema_name: &Option<String>) -> Result<NodeRef<Expr>>;
}
Expand Down Expand Up @@ -71,8 +73,8 @@ impl ExprGenerator<serde_yaml::Value> for ExprBuilder {
serde_yaml::Value::Bool(j_bool) => {
let name_const = match NameConstant::try_from(*j_bool) {
Ok(nc) => nc,
Err(_) => {
bail!("Failed to Load Validated File")
Err(err) => {
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}, {err}")
}
};

Expand All @@ -85,7 +87,7 @@ impl ExprGenerator<serde_yaml::Value> for ExprBuilder {
let number_lit = match j_num.as_f64() {
Some(num_f64) => num_f64,
None => {
bail!("Failed to Load Validated File")
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}")
}
};

Expand All @@ -97,7 +99,7 @@ impl ExprGenerator<serde_yaml::Value> for ExprBuilder {
let number_lit = match j_num.as_i64() {
Some(j_num) => j_num,
None => {
bail!("Failed to Load Validated File")
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}")
}
};

Expand All @@ -106,14 +108,14 @@ impl ExprGenerator<serde_yaml::Value> for ExprBuilder {
value: NumberLitValue::Int(number_lit)
})))
} else {
bail!("Failed to Load Validated File, Unsupported Unsigned 64");
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}, Unsupported Unsigned 64");
}
}
serde_yaml::Value::String(j_string) => {
let str_lit = match StringLit::try_from(j_string.to_string()) {
Ok(s) => s,
Err(_) => {
bail!("Failed to Load Validated File")
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}")
}
};
Ok(node_ref!(Expr::StringLit(str_lit)))
Expand All @@ -123,7 +125,7 @@ impl ExprGenerator<serde_yaml::Value> for ExprBuilder {
for j_arr_item in j_arr {
j_arr_ast_nodes.push(
self.generate(j_arr_item, schema_name)
.with_context(|| "Failed to Load Validated File".to_string())?,
.with_context(|| FAIL_LOAD_VALIDATED_ERR_MSG)?,
);
}
Ok(node_ref!(Expr::List(ListExpr {
Expand All @@ -138,10 +140,10 @@ impl ExprGenerator<serde_yaml::Value> for ExprBuilder {
// The configuration builder already in the schema no longer needs a schema name
let k = self
.generate(k, &None)
.with_context(|| "Failed to Load Validated File".to_string())?;
.with_context(|| FAIL_LOAD_VALIDATED_ERR_MSG)?;
let v = self
.generate(v, &None)
.with_context(|| "Failed to Load Validated File".to_string())?;
.with_context(|| FAIL_LOAD_VALIDATED_ERR_MSG)?;

let config_entry = node_ref!(ConfigEntry {
key: Some(k),
Expand Down Expand Up @@ -173,8 +175,11 @@ impl ExprGenerator<serde_yaml::Value> for ExprBuilder {
None => Ok(config_expr),
}
}
serde_yaml::Value::Tagged(_) => {
bail!("Failed to Load Validated File, Unsupported Yaml Tagged.")
serde_yaml::Value::Tagged(v) => {
bail!(
"{FAIL_LOAD_VALIDATED_ERR_MSG}, Unsupported Yaml tag {}",
v.tag
)
}
}
}
Expand All @@ -193,8 +198,8 @@ impl ExprGenerator<serde_json::Value> for ExprBuilder {
serde_json::Value::Bool(j_bool) => {
let name_const = match NameConstant::try_from(*j_bool) {
Ok(nc) => nc,
Err(_) => {
bail!("Failed to Load Validated File")
Err(err) => {
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}, {err}")
}
};

Expand All @@ -207,7 +212,7 @@ impl ExprGenerator<serde_json::Value> for ExprBuilder {
let number_lit = match j_num.as_f64() {
Some(num_f64) => num_f64,
None => {
bail!("Failed to Load Validated File")
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}")
}
};

Expand All @@ -219,7 +224,7 @@ impl ExprGenerator<serde_json::Value> for ExprBuilder {
let number_lit = match j_num.as_i64() {
Some(j_num) => j_num,
None => {
bail!("Failed to Load Validated File")
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}")
}
};

Expand All @@ -228,14 +233,14 @@ impl ExprGenerator<serde_json::Value> for ExprBuilder {
value: NumberLitValue::Int(number_lit)
})))
} else {
bail!("Failed to Load Validated File, Unsupported Unsigned 64");
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}, Unsupported Unsigned 64");
}
}
serde_json::Value::String(j_string) => {
let str_lit = match StringLit::try_from(j_string.to_string()) {
Ok(s) => s,
Err(_) => {
bail!("Failed to Load Validated File")
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}")
}
};

Expand All @@ -246,7 +251,7 @@ impl ExprGenerator<serde_json::Value> for ExprBuilder {
for j_arr_item in j_arr {
j_arr_ast_nodes.push(
self.generate(j_arr_item, schema_name)
.with_context(|| "Failed to Load Validated File".to_string())?,
.with_context(|| FAIL_LOAD_VALIDATED_ERR_MSG)?,
);
}
Ok(node_ref!(Expr::List(ListExpr {
Expand All @@ -260,13 +265,13 @@ impl ExprGenerator<serde_json::Value> for ExprBuilder {
for (k, v) in j_map.iter() {
let k = match StringLit::try_from(k.to_string()) {
Ok(s) => s,
Err(_) => {
bail!("Failed to Load Validated File")
Err(err) => {
bail!("{FAIL_LOAD_VALIDATED_ERR_MSG}, {err}")
}
};
let v = self
.generate(v, &None)
.with_context(|| "Failed to Load Validated File".to_string())?;
.with_context(|| FAIL_LOAD_VALIDATED_ERR_MSG)?;

let config_entry = node_ref!(ConfigEntry {
key: Some(node_ref!(Expr::StringLit(k))),
Expand Down
6 changes: 3 additions & 3 deletions kclvm/tools/src/vet/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ mod test_expr_builder {
panic!("unreachable")
}
Err(err) => {
assert_eq!(format!("{:?}", err), "Failed to Load JSON\n\nCaused by:\n 0: Failed to Load Validated File\n 1: Failed to Load Validated File, Unsupported Unsigned 64");
assert_eq!(format!("{:?}", err), "Failed to Load JSON\n\nCaused by:\n 0: Failed to load the validated file\n 1: Failed to load the validated file, Unsupported Unsigned 64");
}
};
}
Expand All @@ -318,7 +318,7 @@ mod test_expr_builder {
panic!("unreachable")
}
Err(err) => {
assert_eq!(format!("{:?}", err), "Failed to Load YAML\n\nCaused by:\n 0: Failed to Load Validated File\n 1: Failed to Load Validated File, Unsupported Unsigned 64");
assert_eq!(format!("{:?}", err), "Failed to Load YAML\n\nCaused by:\n 0: Failed to load the validated file\n 1: Failed to load the validated file, Unsupported Unsigned 64");
}
};
}
Expand All @@ -333,7 +333,7 @@ mod test_expr_builder {
panic!("unreachable")
}
Err(err) => {
assert_eq!(format!("{:?}", err), "Failed to Load YAML\n\nCaused by:\n Failed to Load Validated File, Unsupported Yaml Tagged.");
assert_eq!(format!("{:?}", err), "Failed to Load YAML\n\nCaused by:\n Failed to load the validated file, Unsupported Yaml tag !mytag");
}
};
}
Expand Down
27 changes: 8 additions & 19 deletions kclvm/tools/src/vet/validator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,38 +176,27 @@ pub fn validate(val_opt: ValidateOption) -> Result<bool, String> {
None => TMP_FILE.to_string(),
};

let mut module: Module = match kclvm_parser::parse_file(&k_path, val_opt.kcl_code) {
Ok(ast_m) => ast_m,
Err(err_msg) => return Err(err_msg),
};
let mut module = kclvm_parser::parse_file(&k_path, val_opt.kcl_code)?;

let schemas = filter_schema_stmt(&module);
let schema_name = match val_opt.schema_name {
Some(name) => Some(name),
None => schemas.get(0).map(|schema| schema.name.node.clone()),
};

let expr_builder = match ExprBuilder::new_with_file_path(
val_opt.validated_file_kind,
val_opt.validated_file_path,
) {
Ok(builder) => builder,
Err(_) => return Err("Failed to load validated file.".to_string()),
};
let expr_builder =
ExprBuilder::new_with_file_path(val_opt.validated_file_kind, val_opt.validated_file_path)
.map_err(|_| "Failed to load validated file.".to_string())?;

let validated_expr = match expr_builder.build(schema_name) {
Ok(expr) => expr,
Err(_) => return Err("Failed to load validated file.".to_string()),
};
let validated_expr = expr_builder
.build(schema_name)
.map_err(|_| "Failed to load validated file.".to_string())?;

let assign_stmt = build_assign(&val_opt.attribute_name, validated_expr);

module.body.insert(0, assign_stmt);

match execute_module(module) {
Ok(_) => Ok(true),
Err(err) => Err(err),
}
execute_module(module).map(|_| true)
}

fn build_assign(attr_name: &str, node: NodeRef<Expr>) -> NodeRef<Stmt> {
Expand Down

0 comments on commit cde5471

Please sign in to comment.