From 6024a001b48cff632faa6406f954a027982d6750 Mon Sep 17 00:00:00 2001 From: JT Date: Wed, 13 Oct 2021 06:44:23 +1300 Subject: [PATCH] Clarify todo/fixmes --- crates/nu-cli/src/completions.rs | 4 +++- .../nu-command/src/conversions/into/binary.rs | 2 +- crates/nu-command/src/default_context.rs | 2 +- crates/nu-command/src/experimental/git.rs | 4 ++-- .../nu-command/src/experimental/git_checkout.rs | 4 ++-- crates/nu-command/src/filesystem/mv.rs | 2 +- crates/nu-command/src/filters/each.rs | 3 +-- crates/nu-engine/src/documentation.rs | 2 +- crates/nu-engine/src/from_value.rs | 13 ------------- crates/nu-parser/src/errors.rs | 3 +-- crates/nu-parser/src/parse_keywords.rs | 8 ++++---- crates/nu-parser/src/parser.rs | 17 ++++++++--------- crates/nu-protocol/src/ast/expr.rs | 2 +- crates/nu-protocol/src/ast/expression.rs | 2 +- .../src/engine/evaluation_context.rs | 4 ++-- crates/nu-protocol/src/syntax_shape.rs | 2 +- crates/nu-protocol/src/value/range.rs | 2 +- crates/nu-protocol/src/value/stream.rs | 1 - 18 files changed, 31 insertions(+), 46 deletions(-) diff --git a/crates/nu-cli/src/completions.rs b/crates/nu-cli/src/completions.rs index 57298d0936..5a7009eb2e 100644 --- a/crates/nu-cli/src/completions.rs +++ b/crates/nu-cli/src/completions.rs @@ -82,7 +82,9 @@ impl Completer for NuCompleter { Ok(Value::List { vals, .. }) => vals .into_iter() .map(move |x| { - let s = x.as_string().expect("FIXME"); + let s = x.as_string().expect( + "FIXME: better error handling for custom completions", + ); ( reedline::Span { diff --git a/crates/nu-command/src/conversions/into/binary.rs b/crates/nu-command/src/conversions/into/binary.rs index c4161fa4e8..4778d1c07f 100644 --- a/crates/nu-command/src/conversions/into/binary.rs +++ b/crates/nu-command/src/conversions/into/binary.rs @@ -93,7 +93,7 @@ fn into_binary( input.map(head, move |v| { action(v, head) - // FIXME: Add back in column path support + // FIXME: Add back in cell_path support // if column_paths.is_empty() { // action(v, head) // } else { diff --git a/crates/nu-command/src/default_context.rs b/crates/nu-command/src/default_context.rs index f691ff59bf..d36d51f67f 100644 --- a/crates/nu-command/src/default_context.rs +++ b/crates/nu-command/src/default_context.rs @@ -22,7 +22,7 @@ pub fn create_default_context() -> Rc> { }; } - // TODO: sort items categorically + // TODO: sort default context items categorically bind_command!( Alias, Benchmark, diff --git a/crates/nu-command/src/experimental/git.rs b/crates/nu-command/src/experimental/git.rs index 5fe8521f39..eddb042bc7 100644 --- a/crates/nu-command/src/experimental/git.rs +++ b/crates/nu-command/src/experimental/git.rs @@ -37,13 +37,13 @@ impl Command for Git { Ok(Value::string(&String::from_utf8_lossy(&result), call.head)) } Err(_err) => { - // FIXME + // FIXME: Move this to an external signature and add better error handling Ok(Value::nothing()) } } } Err(_err) => { - // FIXME + // FIXME: Move this to an external signature and add better error handling Ok(Value::nothing()) } } diff --git a/crates/nu-command/src/experimental/git_checkout.rs b/crates/nu-command/src/experimental/git_checkout.rs index 143fff96b3..84cd0e5d33 100644 --- a/crates/nu-command/src/experimental/git_checkout.rs +++ b/crates/nu-command/src/experimental/git_checkout.rs @@ -52,13 +52,13 @@ impl Command for GitCheckout { Ok(Value::string(&String::from_utf8_lossy(&result), call.head)) } Err(_err) => { - // FIXME + // FIXME: Move this to an external signature and add better error handling Ok(Value::nothing()) } } } Err(_err) => { - // FIXME + // FIXME: Move this to an external signature and add better error handling Ok(Value::nothing()) } } diff --git a/crates/nu-command/src/filesystem/mv.rs b/crates/nu-command/src/filesystem/mv.rs index 19cdf0d896..98dd8a3217 100644 --- a/crates/nu-command/src/filesystem/mv.rs +++ b/crates/nu-command/src/filesystem/mv.rs @@ -37,7 +37,7 @@ impl Command for Mv { call: &Call, _input: Value, ) -> Result { - // TODO: handle invalid directory or insufficient permissions + // TODO: handle invalid directory or insufficient permissions when moving let source: String = call.req(context, 0)?; let destination: String = call.req(context, 1)?; diff --git a/crates/nu-command/src/filters/each.rs b/crates/nu-command/src/filters/each.rs index 39303d005d..f2e6de61f1 100644 --- a/crates/nu-command/src/filters/each.rs +++ b/crates/nu-command/src/filters/each.rs @@ -217,7 +217,7 @@ impl Command for Each { Value::Record { mut cols, mut vals, .. } => { - // TODO check that the lengths match + // TODO check that the lengths match when traversing record output_cols.append(&mut cols); output_vals.append(&mut vals); } @@ -235,7 +235,6 @@ impl Command for Each { }) } x => { - //TODO: we need to watch to make sure this is okay let engine_state = context.engine_state.borrow(); let block = engine_state.get_block(block_id); diff --git a/crates/nu-engine/src/documentation.rs b/crates/nu-engine/src/documentation.rs index b0269f340b..928320fc26 100644 --- a/crates/nu-engine/src/documentation.rs +++ b/crates/nu-engine/src/documentation.rs @@ -6,7 +6,7 @@ const COMMANDS_DOCS_DIR: &str = "docs/commands"; pub struct DocumentationConfig { no_subcommands: bool, - //FIXME: + //FIXME: add back in color support #[allow(dead_code)] no_color: bool, brief: bool, diff --git a/crates/nu-engine/src/from_value.rs b/crates/nu-engine/src/from_value.rs index d878dec9b3..5dc0b517f3 100644 --- a/crates/nu-engine/src/from_value.rs +++ b/crates/nu-engine/src/from_value.rs @@ -100,19 +100,6 @@ impl FromValue for Spanned { } } -//FIXME -/* -impl FromValue for ColumnPath { - fn from_value(v: &Value) -> Result { - match v { - Value:: => Ok(c.clone()), - v => Err(ShellError::type_error("column path", v.spanned_type_name())), - } - } -} - -*/ - impl FromValue for CellPath { fn from_value(v: &Value) -> Result { let span = v.span()?; diff --git a/crates/nu-parser/src/errors.rs b/crates/nu-parser/src/errors.rs index 3c92ca829d..36412e9a7b 100644 --- a/crates/nu-parser/src/errors.rs +++ b/crates/nu-parser/src/errors.rs @@ -89,8 +89,7 @@ pub enum ParseError { #[diagnostic( code(nu::parser::unknown_command), url(docsrs), - // TODO: actual suggestions - // help("Did you mean `foo`?") + // TODO: actual suggestions like "Did you mean `foo`?" )] UnknownCommand(#[label = "unknown command"] Span), diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 05fe97e5eb..7f405714dd 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -110,7 +110,7 @@ pub fn parse_def( *declaration = signature.into_block_command(block_id); } else { error = error.or_else(|| { - // TODO: Add InternalError variant + // FIXME: add a variant to ParseError that represents internal errors Some(ParseError::UnknownState( "Internal error: Predeclaration failed to add declaration" .into(), @@ -307,7 +307,7 @@ pub fn parse_export( ( garbage_statement(spans), Some(ParseError::UnknownState( - // TODO: fill in more as they come + // TODO: fill in more export types as they come "Expected structure: export def [] {}".into(), span(spans), )), @@ -400,7 +400,7 @@ pub fn parse_module( let name = working_set.get_span_contents(pipeline.commands[0].parts[0]); let (stmt, err) = match name { - // TODO: Here we can add other stuff that's alowed for modules + // TODO: Here we can add other stuff that's allowed for modules b"def" => { let (stmt, err) = parse_def(working_set, &pipeline.commands[0].parts); @@ -427,7 +427,7 @@ pub fn parse_module( _ => ( garbage_statement(&pipeline.commands[0].parts), Some(ParseError::Expected( - // TODO: Fill in more as they com + // TODO: Fill in more keywords as they come "def or export keyword".into(), pipeline.commands[0].parts[0], )), diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index e4eb9f489f..7a56d41e42 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -149,7 +149,7 @@ fn parse_long_flag( let arg_contents = working_set.get_span_contents(arg_span); if arg_contents.starts_with(b"--") { - // FIXME: only use the first you find + // FIXME: only use the first flag you find? let split: Vec<_> = arg_contents.split(|x| *x == b'=').collect(); let long_name = String::from_utf8(split[0].into()); if let Ok(long_name) = long_name { @@ -580,7 +580,7 @@ pub fn parse_internal_call( working_set.exit_scope(); } - // FIXME: type unknown + // FIXME: output type unknown (Box::new(call), span(spans), error) } @@ -728,7 +728,7 @@ pub fn parse_call( Expression { expr: Expr::Call(call), span: span(spans), - ty: Type::Unknown, // FIXME + ty: Type::Unknown, // FIXME: calls should have known output types custom_completion: None, }, err, @@ -922,7 +922,7 @@ pub fn parse_range( // Now, based on the operator positions, figure out where the bounds & next are located and // parse them - // TODO: Actually parse the next number + // TODO: Actually parse the next number in the range let from = if token.starts_with("..") { // token starts with either next operator, or range operator -- we don't care which one None @@ -1260,7 +1260,6 @@ pub fn parse_full_cell_path( implicit_head: Option, span: Span, ) -> (Expression, Option) { - // FIXME: assume for now a paren expr, but needs more let full_cell_span = span; let source = working_set.get_span_contents(span); let mut error = None; @@ -1642,7 +1641,7 @@ pub fn parse_string( } } -//TODO: Handle error case +//TODO: Handle error case for unknown shapes pub fn parse_shape_name( _working_set: &StateWorkingSet, bytes: &[u8], @@ -1657,7 +1656,7 @@ pub fn parse_shape_name( b"int" => SyntaxShape::Int, b"path" => SyntaxShape::Filepath, b"glob" => SyntaxShape::GlobPattern, - b"block" => SyntaxShape::Block(None), //FIXME + b"block" => SyntaxShape::Block(None), //FIXME: Blocks should have known output types b"cond" => SyntaxShape::RowCondition, b"operator" => SyntaxShape::Operator, b"math" => SyntaxShape::MathExpression, @@ -2161,7 +2160,7 @@ pub fn parse_signature_helper( let (syntax_shape, err) = parse_shape_name(working_set, contents, span); error = error.or(err); - //TODO check if we're replacing one already + //TODO check if we're replacing a custom parameter already match last { Arg::Positional(PositionalArg { shape, var_id, .. }, ..) => { working_set.set_variable_type(var_id.expect("internal error: all custom parameters must have var_ids"), syntax_shape.to_type()); @@ -2555,7 +2554,7 @@ pub fn parse_block_expression( if let Some(signature) = signature { output.signature = signature; } else if let Some(last) = working_set.delta.scope.last() { - // FIXME: this only supports the top $it. Instead, we should look for a free $it in the expression. + // FIXME: this only supports the top $it. Is this sufficient? if let Some(var_id) = last.get_var(b"$it") { let mut signature = Signature::new(""); diff --git a/crates/nu-protocol/src/ast/expr.rs b/crates/nu-protocol/src/ast/expr.rs index 718dec5f6a..e1baae1ac1 100644 --- a/crates/nu-protocol/src/ast/expr.rs +++ b/crates/nu-protocol/src/ast/expr.rs @@ -26,7 +26,7 @@ pub enum Expr { ValueWithUnit(Box, Spanned), Filepath(String), GlobPattern(String), - String(String), // FIXME: improve this in the future? + String(String), CellPath(CellPath), FullCellPath(Box), Signature(Box), diff --git a/crates/nu-protocol/src/ast/expression.rs b/crates/nu-protocol/src/ast/expression.rs index da79d9cc9d..5627b167c2 100644 --- a/crates/nu-protocol/src/ast/expression.rs +++ b/crates/nu-protocol/src/ast/expression.rs @@ -39,7 +39,7 @@ impl Expression { | Operator::In | Operator::NotIn => 80, Operator::And => 50, - Operator::Or => 40, // TODO: should we have And and Or be different precedence? + Operator::Or => 40, } } _ => 0, diff --git a/crates/nu-protocol/src/engine/evaluation_context.rs b/crates/nu-protocol/src/engine/evaluation_context.rs index 07a71ff91a..b376294047 100644 --- a/crates/nu-protocol/src/engine/evaluation_context.rs +++ b/crates/nu-protocol/src/engine/evaluation_context.rs @@ -25,8 +25,8 @@ impl EvaluationContext { // We need to make values concreate before we assign them to variables, as stream values // will drain and remain drained. // - // TODO: find a good home for this - // TODO: add ctrl-c support + // TODO: find a good home for converting a stream->list when storing into a variable + // TODO: add ctrl-c support when setting a var let value = match value { Value::Stream { stream, span } => Value::List { diff --git a/crates/nu-protocol/src/syntax_shape.rs b/crates/nu-protocol/src/syntax_shape.rs index 7f058e18e1..ca442eae58 100644 --- a/crates/nu-protocol/src/syntax_shape.rs +++ b/crates/nu-protocol/src/syntax_shape.rs @@ -108,7 +108,7 @@ impl SyntaxShape { SyntaxShape::Boolean => Type::Bool, SyntaxShape::Signature => Type::Unknown, SyntaxShape::String => Type::String, - SyntaxShape::Table => Type::List(Box::new(Type::Unknown)), // FIXME + SyntaxShape::Table => Type::List(Box::new(Type::Unknown)), // FIXME: Tables should have better types SyntaxShape::VarWithOptType => Type::Unknown, SyntaxShape::Variable => Type::Unknown, } diff --git a/crates/nu-protocol/src/value/range.rs b/crates/nu-protocol/src/value/range.rs index 14bae6ea13..24e6e6b7b8 100644 --- a/crates/nu-protocol/src/value/range.rs +++ b/crates/nu-protocol/src/value/range.rs @@ -24,7 +24,7 @@ impl Range { operator: &RangeOperator, ) -> Result { // Select from & to values if they're not specified - // TODO: Replace the placeholder values with proper min/max based on data type + // TODO: Replace the placeholder values with proper min/max for range based on data type let from = if let Value::Nothing { .. } = from { Value::Int { val: 0i64, diff --git a/crates/nu-protocol/src/value/stream.rs b/crates/nu-protocol/src/value/stream.rs index d3774ca750..87ab2fec71 100644 --- a/crates/nu-protocol/src/value/stream.rs +++ b/crates/nu-protocol/src/value/stream.rs @@ -62,7 +62,6 @@ impl<'de> Deserialize<'de> for ValueStream { where D: serde::Deserializer<'de>, { - // FIXME: implement these deserializer.deserialize_seq(MySeqVisitor) } }