Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mismatches between help messages and actual behaviour #9573

Open
17 tasks
baehyunsol opened this issue Jun 30, 2023 · 13 comments
Open
17 tasks

Mismatches between help messages and actual behaviour #9573

baehyunsol opened this issue Jun 30, 2023 · 13 comments
Labels
documentation issues relating to documentation enhancement New feature or request good first issue Good for newcomers help Related to help commands and pages meta-issue An issue that tracks other issues

Comments

@baehyunsol
Copy link
Contributor

baehyunsol commented Jun 30, 2023

Related problem

After issuing #9568, I read other help messages and their implementations. I found many other mismatches between the help messages and actual behaviours. Some of them are too trivial to mention in help messages, (for example, a range is treated like list<int> ) but some have odd behaviours. Below is the list of what I found. It's not exhaustive though: I haven't tested all the commands.

  • parse
    • the help message says it only works for strings
    • 0x[66 6f 6f 20 62 61 72] | parse "{a} {b}" also works
  • input list
    • the help message says it only works for list
    • it works for range, list, and record
  • split list
    • the help message says it only works for list
    • it basically works on any type: true | split list "r" is [[true]]
  • size, split chars, split words and split column
    • the help message says it only works for strings
    • it works on any type that implements .as_string()
    • 200 | size, 200 | split chars, 2010 | split column "1", and 200 | split words works
  • all the math commands
    • if the help message says they only work for list: they work on records, ranges, and lists
      • I have no idea how they work with records. {a: 1, b: 2} | math stddev is {a: 0, b: 0}, and {a: [1], b: [2]} | math stddev is an error
    • if the help message says they only work for numbers, they also work on list, like a map function
  • hash md5 and hash sha256
    • the help message says it only works for strings
    • it works on binary, list, and list
  • str substring
    • the help message says it only works for strings
    • it works on strings and list
  • transpose
    • the help message says it only works for tables
    • it seems like it works on every type! true | transpose is an empty list, but I have no idea what that means
  • zip
    • the help message says it works for lists and ranges
    • it seems like it works on every type! if a scalar value, like true is given, it converts that to a list, like [true]
  • sort
    • the help message says it works for lists and records
    • it works on any type! if a scalar value, like true is given, it does nothing and returns true.
  • reject
    • the help message says it works for records and tables
    • it works on any type if no argument is given. for example, false | reject is false, but I have no idea what that means
  • prepend and append
    • the help message says it only works for list
    • it works on any type. if a scalar value, like true is given, it converts that to a list. for example, true | prepend false is [false, true]
  • select
    • the help message says it only works for records and tables
    • it works on any type. if a scalar value, like true is given, it converts that to a list. for example, true | select 0 is [true]
  • find
    • the help message says it only works for lists, strings and tables
    • it works on any type. it returns null if a scalar value is given. for example, 200 | find and 200 | find 0 are both null.
    • also, it would be nicer if the help message tells us what happens when no terms are given.
  • first
    • the help message says it works for lists and binaries
    • it works on ranges, but that's too obvious
    • it also works on tables, but the help message doesn't mention that
  • last
    • the help message says it works for lists
    • it works on types that implements .into_iter_strict, which are lists, binaries, ranges, and streams
    • it also works on tables, but the help message doesn't mention that
  • drop column
    • the help message says it only works for tables
    • it works on any type. for example, 200 | drop column and true | drop column are both empty records.

It seems like most mismatches are from v.as_string(), v.into_iter() and input.into_iter(), where v is a Value and input is a PipelineData. They cause strange behaviours like 200 | find or true | select 0.

Describe the solution you'd like

Below are the behaviours that are in the above list and look odd to me.

  • true | split list "r" is [[true]]
  • {a: 1, b: 2} | math stddev is {a: 0.00, b: 0.00}
  • true | transpose is {}
  • true | sort is true
  • true | reject is true
  • 200 | find is null
  • true | drop column is {}

Describe alternatives you've considered

No response

Additional context and details

No response

@baehyunsol baehyunsol added enhancement New feature or request needs-triage An issue that hasn't had any proper look labels Jun 30, 2023
@fdncred
Copy link
Collaborator

fdncred commented Jun 30, 2023

nice list. we'd love a PR to straighten this out.

@amtoine amtoine added good first issue Good for newcomers documentation issues relating to documentation meta-issue An issue that tracks other issues help Related to help commands and pages and removed needs-triage An issue that hasn't had any proper look labels Jun 30, 2023
@amtoine
Copy link
Member

amtoine commented Jun 30, 2023

very nice, thanks 😋

@baehyunsol
Copy link
Contributor Author

I updated the comment: I didn't know that table was a pretty-printer for tables. I thought it was some kind of into table command.

@sholderbach sholderbach pinned this issue Jul 5, 2023
@J-Kappes
Copy link
Contributor

I don't think it makes sense to say that such commands that only work on lists will work on everything just because they convert their input into a list if it isn't one already. Sure, they don't throw errors but they also don't serve their actual purpose when sorting a list of length 1 etc.

Similarly, adding that they work on tables is technically redundant because tables are lists of records, but I see the pedagogical value in adding "including tables" wherever lists of any kind are accepted.

@dbofmmbt
Copy link

dbofmmbt commented Sep 30, 2023

Regarding the parse command, the example in the issue doesn't work anymore.

Error: nu::parser::input_type_mismatch

  × Command does not support binary input.
   ╭─[entry #30:1:1]
 1 │ 0x[66 6f 6f 20  62 61 72] | parse "{a} {b}"
   ·                             ──┬──
   ·                               ╰── command doesn't support binary input
   ╰────

In fact, several commands now complain about wrong types.

@fdncred
Copy link
Collaborator

fdncred commented Sep 30, 2023

@dbofmmbt This is because we made our type system more strict with commands. If you look at commands, they have input_output_types() now. I'm guessing the reason that parse doesn't work is because it doesn't have Binary as one of those types.

If you see a bunch of commands with these errors, like you're indicating, please file issues for each command because we'd want to fix those.

I added this to the signature for parse

            .input_output_types(vec![
                (Type::String, Type::Table(vec![])),
                (Type::List(Box::new(Type::Any)), Type::Table(vec![])),
                (Type::Binary, Type::Table(vec![])),
            ])

and can now do this

 0x[66 6f 6f 20  62 61 72] | parse "{a} {b}"
#┬─a─┬─b─╮
│0│foo│bar│
╰─┴───┴───╯

Feel free to submit a PR with these changes and an example.

@quat1024
Copy link
Contributor

quat1024 commented Oct 10, 2023

table's examples for --expand and --collapse do not actually show them working:

  Render data in table view (expanded)
  > [[a b]; [1 2] [2 [4 4]]] | table --expand
  ╭───┬───┬───╮
  │ # │ a │ b │
  ├───┼───┼───┤
  │ 0 │ 1 │ 2 │
  │ 1 │ 3 │ 4 │
  ╰───┴───┴───╯

  Render data in table view (collapsed)
  > [[a b]; [1 2] [2 [4 4]]] | table --collapse
  ╭───┬───┬───╮
  │ # │ a │ b │
  ├───┼───┼───┤
  │ 0 │ 1 │ 2 │
  │ 1 │ 3 │ 4 │
  ╰───┴───┴───╯

0.85.0

@fdncred
Copy link
Collaborator

fdncred commented Oct 10, 2023

That seems to be a bug in our help rendering. Probably should file a separate issue on that.

@KAAtheWiseGit
Copy link
Contributor

All of the examples now return a command doesn't support {type} input error.

@amtoine
Copy link
Member

amtoine commented Dec 1, 2023

All of the examples now return a command doesn't support {type} input error.

does that mean that this issue is addressed? 😮

@KAAtheWiseGit
Copy link
Contributor

KAAtheWiseGit commented Dec 1, 2023

I seems so, yeah.

@amtoine
Copy link
Member

amtoine commented Dec 2, 2023

thanks @KAAtheWiseGit, it looks like i can't reproduce these examples in 0.87.1 👍
i think the PR that introduced the fix was #9680 👌

please feel free to ask for a reopen if some still don't work 😉

@amtoine amtoine closed this as completed Dec 2, 2023
@sholderbach
Copy link
Member

@amtoine / @KAAtheWiseGit not sure if I agree on the asessment at face value. The input/output type pairs mentioned in the help text are the same that generate (with some potential technical discrepancies if ambiguous) the type check at parse time.
This creates the command doesn't support ... input error i.e. ParseError::InputMismatch.
This is not strongly linked to what the internal Command::run implementation can provide. If the parse stage returns Type::Any the error would not be raised and potentially additional paths could be exercised, as also visible in originally provided examples (check necessary if they are still valid). Thanks to @baehyunsol for doing the work of noting the discrepancies. But this needs due diligence in the details of the commands and maybe some tweaking to the documentation or input/output types.

@sholderbach sholderbach reopened this Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation issues relating to documentation enhancement New feature or request good first issue Good for newcomers help Related to help commands and pages meta-issue An issue that tracks other issues
Projects
Status: Done
Development

No branches or pull requests

8 participants