Merge pull request #133 from nushell/todo_fixme

Clarify todo/fixmes

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 {

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 {

crates/nu-command/src/default_context.rs

@@ -22,7 +22,7 @@ pub fn create_default_context() -> Rc<RefCell<EngineState>> {
             };
         }
 
-        // TODO: sort items categorically
+        // TODO: sort default context items categorically
         bind_command!(
             Alias,
             Benchmark,

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())
             }
         }

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())
             }
         }

crates/nu-command/src/filesystem/mv.rs

@@ -37,7 +37,7 @@ impl Command for Mv {
         call: &Call,
         _input: Value,
     ) -> Result<nu_protocol::Value, nu_protocol::ShellError> {
-        // 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)?;
 

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);
 

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,

crates/nu-engine/src/from_value.rs

@@ -100,19 +100,6 @@ impl FromValue for Spanned<String> {
     }
 }
 
-//FIXME
-/*
-impl FromValue for ColumnPath {
-    fn from_value(v: &Value) -> Result<Self, ShellError> {
-        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<Self, ShellError> {
         let span = v.span()?;

crates/nu-parser/src/errors.rs

@@ -89,8 +89,7 @@ pub enum ParseError {
     #[diagnostic(
         code(nu::parser::unknown_command),
         url(docsrs),
-        // TODO: actual suggestions
+        // TODO: actual suggestions like "Did you mean `foo`?"
-        // help("Did you mean `foo`?")
     )]
     UnknownCommand(#[label = "unknown command"] Span),
 

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],
                             )),

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<VarId>,
     span: Span,
 ) -> (Expression, Option<ParseError>) {
-    // 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("");

crates/nu-protocol/src/ast/expr.rs

@@ -26,7 +26,7 @@ pub enum Expr {
     ValueWithUnit(Box<Expression>, Spanned<Unit>),
     Filepath(String),
     GlobPattern(String),
-    String(String), // FIXME: improve this in the future?
+    String(String),
     CellPath(CellPath),
     FullCellPath(Box<FullCellPath>),
     Signature(Box<Signature>),

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,

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: find a good home for converting a stream->list when storing into a variable
-        // TODO: add ctrl-c support
+        // TODO: add ctrl-c support when setting a var
 
         let value = match value {
             Value::Stream { stream, span } => Value::List {

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,
         }

crates/nu-protocol/src/value/range.rs

@@ -24,7 +24,7 @@ impl Range {
         operator: &RangeOperator,
     ) -> Result<Range, ShellError> {
         // 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,

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)
     }
 }