Skip to content

Commit

Permalink
feat(CLI): did you mean for struct values
Browse files Browse the repository at this point in the history
* functionality is cursor-aware, and fixes the actual string the user
  passed in. That way, it is made very clear how the suggested value
  is to be used.
* it's a known weakness of the implementation that it operates on a
  flattened list of field names, and thus may make nonsensical
  suggestions.
* added punctuation to all errors

Fixes #67
[skip ci]
  • Loading branch information
Byron committed May 2, 2015
1 parent d2a4e2f commit 96415d1
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 30 deletions.
1 change: 1 addition & 0 deletions etc/api/type-cli.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ cargo:
is_executable: YES
dependencies:
- clap = "*"
- strsim = "*"
- yup-hyper-mock = "*"
- serde = ">= 0.3.0"
- serde_macros = "*"
4 changes: 2 additions & 2 deletions src/mako/cli/lib/argparse.mako
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ let arg_data = [
if mc.request_value:
args.append((
STRUCT_FLAG,
"Set various fields of the request structure",
"Set various fields of the request structure, matching the key=value form",
KEY_VALUE_ARG,
True,
True,
Expand All @@ -198,7 +198,7 @@ let arg_data = [
if mc.optional_props or parameters is not UNDEFINED:
args.append((
PARAM_FLAG,
"Set various fields of the request structure",
"Set various optional parameters, matching the key=value form",
VALUE_ARG,
False,
True,
Expand Down
4 changes: 4 additions & 0 deletions src/mako/cli/lib/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ def new_method_context(resource, method, c):
return MethodContext(m, response_schema, params, request_value, media_params,
required_props, optional_props, part_prop)

# Returns a string representing a string-vector of mangled names
# fields is an iterator
def field_vec(fields):
return "vec![%s]" % ', '.join('"%s"' % mangle_subcommand(f) for f in fields)

def pretty(n):
return ' '.join(s.capitalize() for s in mangle_subcommand(n).split('-'))
Expand Down
15 changes: 9 additions & 6 deletions src/mako/cli/lib/engine.mako
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
call_method_ident, POD_TYPES, opt_value, ident, JSON_TYPE_VALUE_MAP,
KEY_VALUE_ARG, to_cli_schema, SchemaEntry, CTYPE_POD, actual_json_type, CTYPE_MAP, CTYPE_ARRAY,
application_secret_path, DEBUG_FLAG, DEBUG_AUTH_FLAG, CONFIG_DIR_FLAG, req_value, MODE_ARG,
opt_values, SCOPE_ARG, CONFIG_DIR_ARG, DEFAULT_MIME)
opt_values, SCOPE_ARG, CONFIG_DIR_ARG, DEFAULT_MIME, field_vec)
v_arg = '<%s>' % VALUE_ARG
SOPT = 'self.opt'
Expand Down Expand Up @@ -259,7 +259,7 @@ ${value_unwrap}\
call = call.${ADD_PARAM_FN}(map.iter().find(|t| t.0 == key).unwrap_or(&("", key)).1, ${value_unwrap})
},
% endif # handle global parameters
_ => err.issues.push(CLIError::UnknownParameter(key.to_string())),
_ => err.issues.push(CLIError::UnknownParameter(key.to_string(), ${field_vec(global_parameter_names)})),
}
}
% endif # handle call parameters
Expand Down Expand Up @@ -333,7 +333,7 @@ if dry_run {
<%
allow_optionals_fn = lambda s: is_schema_with_optionals(schema_markers(s, c, transitive=False))
def flatten_schema_fields(schema, res, init_fn_map, cur=list(), init_call=None):
def flatten_schema_fields(schema, res, init_fn_map, fields, cur=list(), init_call=None):
if len(cur) == 0:
init_call = ''
cur = list()
Expand All @@ -345,6 +345,7 @@ if dry_run {
opt_access = ''
for fn, f in schema.fields.iteritems():
cur.append(['%s%s' % (mangle_ident(fn), opt_access), fn])
fields.add(fn)
if isinstance(f, SchemaEntry):
cur[-1][0] = mangle_ident(fn)
res.append((init_call, schema, f, list(cur)))
Expand All @@ -367,14 +368,15 @@ if dry_run {
init_fn_map[init_fn_name] = init_fn
# end handle init
flatten_schema_fields(f, res, init_fn_map, cur, init_call)
flatten_schema_fields(f, res, init_fn_map, fields, cur, init_call)
cur.pop()
# endfor
# end utility
schema_fields = list()
init_fn_map = dict()
flatten_schema_fields(request_cli_schema, schema_fields, init_fn_map)
fields = set()
flatten_schema_fields(request_cli_schema, schema_fields, init_fn_map, fields)
%>\
let mut ${request_prop_name} = api::${request_prop_type}::default();
let mut field_cursor = FieldCursor::default();
Expand Down Expand Up @@ -443,7 +445,8 @@ ${opt_suffix}\
},
% endfor # each nested field
_ => {
err.issues.push(CLIError::Field(FieldError::Unknown(temp_cursor.to_string())));
let suggestion = FieldCursor::did_you_mean(key, &${field_vec(sorted(fields))});
err.issues.push(CLIError::Field(FieldError::Unknown(temp_cursor.to_string(), suggestion, value.map(|v| v.to_string()))));
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/mako/cli/main.rs.mako
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ extern crate yup_hyper_mock as mock;
extern crate serde;
extern crate hyper;
extern crate mime;
extern crate strsim;
extern crate ${to_extern_crate_name(library_to_crate_name(library_name(name, version), make.depends_on_suffix))} as api;

use std::env;
Expand Down
122 changes: 100 additions & 22 deletions src/rust/cli/cmn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use oauth2::{ApplicationSecret, ConsoleApplicationSecret, TokenStorage, Token};
use serde::json;
use mime::Mime;
use clap::{App, SubCommand};
use strsim;

use std::fs;
use std::env;
Expand All @@ -16,6 +17,21 @@ use std::default::Default;

const FIELD_SEP: char = '.';

fn did_you_mean<'a>(v: &str, possible_values: &[&'a str]) -> Option<&'a str> {

let mut candidate: Option<(f64, &str)> = None;
for pv in possible_values {
let confidence = strsim::jaro_winkler(v, pv);
if confidence > 0.8 && (candidate.is_none() || (candidate.as_ref().unwrap().0 < confidence)) {
candidate = Some((confidence, pv));
}
}
match candidate {
None => None,
Some((_, candidate)) => Some(candidate),
}
}

pub enum CallType {
Upload(UploadProtocol),
Standard,
Expand Down Expand Up @@ -127,6 +143,49 @@ impl FieldCursor {
Ok(())
}

pub fn did_you_mean(value: &str, possible_values: &[&str]) -> Option<String> {
if value.len() == 0 {
return None
}

let mut last_c = FIELD_SEP;

let mut field = String::new();
let mut output = String::new();

let push_field = |fs: &mut String, f: &mut String| {
if f.len() > 0 {
fs.push_str(
match did_you_mean(&f, possible_values) {
Some(candidate) => candidate,
None => &f,
});
f.truncate(0);
}
};

for (cid, c) in value.chars().enumerate() {
if c == FIELD_SEP {
if last_c != FIELD_SEP {
push_field(&mut output, &mut field);
}
output.push(c);
} else {
field.push(c);
}

last_c = c;
}

push_field(&mut output, &mut field);

if &output == value {
None
} else {
Some(output)
}
}

pub fn num_fields(&self) -> usize {
self.0.len()
}
Expand Down Expand Up @@ -280,10 +339,10 @@ impl fmt::Display for ApplicationSecretError {
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
match *self {
ApplicationSecretError::DecoderError((ref path, ref err))
=> writeln!(f, "Could not decode file at '{}' with error: {}",
=> writeln!(f, "Could not decode file at '{}' with error: {}.",
path, err),
ApplicationSecretError::FormatError(ref path)
=> writeln!(f, "'installed' field is unset in secret file at '{}'",
=> writeln!(f, "'installed' field is unset in secret file at '{}'.",
path),
}
}
Expand All @@ -302,15 +361,15 @@ impl fmt::Display for ConfigurationError {
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
match *self {
ConfigurationError::DirectoryCreationFailed((ref dir, ref err))
=> writeln!(f, "Directory '{}' could not be created with error: {}", dir, err),
=> writeln!(f, "Directory '{}' could not be created with error: {}.", dir, err),
ConfigurationError::DirectoryUnset
=> writeln!(f, "--config-dir was unset or empty"),
=> writeln!(f, "--config-dir was unset or empty."),
ConfigurationError::HomeExpansionFailed(ref dir)
=> writeln!(f, "Couldn't find HOME directory of current user, failed to expand '{}'", dir),
=> writeln!(f, "Couldn't find HOME directory of current user, failed to expand '{}'.", dir),
ConfigurationError::Secret(ref err)
=> writeln!(f, "Secret -> {}", err),
ConfigurationError::IOError((ref path, ref err))
=> writeln!(f, "IO operation failed on path '{}' with error: {}", path, err),
=> writeln!(f, "IO operation failed on path '{}' with error: {}.", path, err),
}
}
}
Expand All @@ -325,9 +384,9 @@ impl fmt::Display for InputError {
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
match *self {
InputError::IOError((ref file_path, ref io_err))
=> writeln!(f, "Failed to open '{}' for reading with error: {}", file_path, io_err),
=> writeln!(f, "Failed to open '{}' for reading with error: {}.", file_path, io_err),
InputError::Mime(ref mime)
=> writeln!(f, "'{}' is not a known mime-type", mime),
=> writeln!(f, "'{}' is not a known mime-type.", mime),
}
}
}
Expand All @@ -336,7 +395,7 @@ impl fmt::Display for InputError {
pub enum FieldError {
PopOnEmpty(String),
TrailingFieldSep(String),
Unknown(String),
Unknown(String, Option<String>, Option<String>),
Empty,
}

Expand All @@ -345,13 +404,26 @@ impl fmt::Display for FieldError {
fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
match *self {
FieldError::PopOnEmpty(ref field)
=> writeln!(f, "'{}': Cannot move up on empty field cursor", field),
=> writeln!(f, "'{}': Cannot move up on empty field cursor.", field),
FieldError::TrailingFieldSep(ref field)
=> writeln!(f, "'{}': Single field separator may not be last character", field),
FieldError::Unknown(ref field)
=> writeln!(f, "Field '{}' does not exist", field),
=> writeln!(f, "'{}': Single field separator may not be last character.", field),
FieldError::Unknown(ref field, ref suggestion, ref value) => {
let suffix =
match *suggestion {
Some(ref s) => {
let kv =
match *value {
Some(ref v) => format!("{}={}", s, v),
None => s.clone(),
};
format!(" Did you mean '{}' ?", kv)
},
None => String::new(),
};
writeln!(f, "Field '{}' does not exist.{}", field, suffix)
},
FieldError::Empty
=> writeln!(f, "Field names must not be empty"),
=> writeln!(f, "Field names must not be empty."),
}
}
}
Expand All @@ -361,7 +433,7 @@ impl fmt::Display for FieldError {
pub enum CLIError {
Configuration(ConfigurationError),
ParseError(&'static str, &'static str, String, String),
UnknownParameter(String),
UnknownParameter(String, Vec<&'static str>),
InvalidUploadProtocol(String, Vec<String>),
InvalidKeyValueSyntax(String, bool),
Input(InputError),
Expand All @@ -377,18 +449,24 @@ impl fmt::Display for CLIError {
CLIError::Input(ref err) => write!(f, "Input -> {}", err),
CLIError::Field(ref err) => write!(f, "Field -> {}", err),
CLIError::InvalidUploadProtocol(ref proto_name, ref valid_names)
=> writeln!(f, "'{}' is not a valid upload protocol. Choose from one of {}", proto_name, valid_names.connect(", ")),
=> writeln!(f, "'{}' is not a valid upload protocol. Choose from one of {}.", proto_name, valid_names.connect(", ")),
CLIError::ParseError(arg_name, type_name, ref value, ref err_desc)
=> writeln!(f, "Failed to parse argument '{}' with value '{}' as {} with error: {}",
=> writeln!(f, "Failed to parse argument '{}' with value '{}' as {} with error: {}.",
arg_name, value, type_name, err_desc),
CLIError::UnknownParameter(ref param_name)
=> writeln!(f, "Parameter '{}' is unknown.", param_name),
CLIError::UnknownParameter(ref param_name, ref possible_values) => {
let mut suffix =
match did_you_mean(param_name, &possible_values) {
Some(v) => format!(" Did you mean '{}' ?", v),
None => String::new(),
};
write!(f, "Parameter '{}' is unknown.{}\n", param_name, suffix)
},
CLIError::InvalidKeyValueSyntax(ref kv, is_hashmap) => {
let hashmap_info = if is_hashmap { "hashmap " } else { "" };
writeln!(f, "'{}' does not match {}pattern <key>=<value>", kv, hashmap_info)
writeln!(f, "'{}' does not match {}pattern <key>=<value>.", kv, hashmap_info)
},
CLIError::MissingCommandError => writeln!(f, "Please specify the main sub-command"),
CLIError::MissingMethodError(ref cmd) => writeln!(f, "Please specify the method to call on the '{}' command", cmd),
CLIError::MissingCommandError => writeln!(f, "Please specify the main sub-command."),
CLIError::MissingMethodError(ref cmd) => writeln!(f, "Please specify the method to call on the '{}' command.", cmd),
}
}
}
Expand Down

0 comments on commit 96415d1

Please sign in to comment.