Skip to content

fix(group-by): re #14337 name collision prevention#14360

Merged
fdncred merged 12 commits intonushell:mainfrom
Bahex:group-by-multiple-naming-1
Nov 17, 2024
Merged

fix(group-by): re #14337 name collision prevention#14360
fdncred merged 12 commits intonushell:mainfrom
Bahex:group-by-multiple-naming-1

Conversation

@Bahex
Copy link
Copy Markdown
Member

@Bahex Bahex commented Nov 16, 2024

A more involved solution to the issue pointed out here

Description

With --to-table

  • cell-path groupers are used to create column names, similar to select

  • closure groupers result in columns named closure_{i} where i is the index of argument, with regards to other closures i.e. first closure grouper results in a column named closure_0

    Previously

    • group-by foo {...} {...} => table<foo, group1, group2, items>
    • group-by {...} foo {...} => table<group0, foo, group2, items>

    With this PR

    • group-by foo {...} {...} => table<foo, closure_0, closure_1, items>
    • group-by {...} foo {...} => table<closure_0, foo, closure_1, items>
  • no grouper argument results in a table<group, items> as previously

On naming conflicts caused by cell-path groupers named items or closure_{i}, an error is thrown, suggesting to use a closure in place of a cell-path.

 ls | rename items | group-by items --to-table 
Error:   × grouper arguments can't be named `items`
   ╭─[entry #3:1:29]
 1 │ ls | rename items | group-by items --to-table 
   ·                             ────────┬────────
   ·                                     ╰── contains `items`
   ╰────
  help: instead of a cell-path, try using a closure

And following the suggestion:

 ls | rename items | group-by { get items } --to-table 
╭─#──┬──────closure_0──────┬───────────────────────────items────────────────────────────╮
 0   CITATION.cff         ╭─#─┬────items─────┬─type─┬─size──┬───modified───╮         
                           0  CITATION.cff  file  812 B  3 months ago          
                          ╰─#─┴────items─────┴─type─┴─size──┴───modified───╯         
 1   CODE_OF_CONDUCT.md   ╭─#─┬───────items────────┬─type─┬──size───┬───modified───╮ 
...

@Bahex Bahex changed the title fix(group-by): re #14337 fix(group-by): re #14337 name collision prevention Nov 16, 2024
@Bahex
Copy link
Copy Markdown
Member Author

Bahex commented Nov 17, 2024

Updated error messages


 ls | rename items | group-by items --to-table
Error:   × grouper arguments can't be named `items`
   ╭─[entry #1:1:30]
 1 │ ls | rename items | group-by items --to-table
   ·                              ──┬──
   ·                                ╰── here
   ╰────
  help: instead of a cell-path, try using a closure: { get items }

 ls | rename closure_0 | group-by {get type} closure_0 --to-table 
Error:   × grouper arguments result in colliding column names
   ╭─[entry #2:1:34]
 1 │ ls | rename closure_0 | group-by {get type} closure_0 --to-table 
   ·                                  ──────────┬─────────
   ·                                            ╰── duplicate column names
   ╰────
  help: instead of a cell-path, try using a closure or renaming columns

Error: nu::shell::column_defined_twice

  × Record field or table column used twice: closure_0
   ╭─[entry #2:1:34]
 1 │ ls | rename closure_0 | group-by {get type} closure_0 --to-table 
   ·                                  ─────┬──── ────┬────
   ·                                       │         ╰── field redefined here
   ·                                       ╰── field first defined here
   ╰────

@fdncred fdncred merged commit e5cec8f into nushell:main Nov 17, 2024
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Nov 17, 2024

Thanks for working on this!

@github-actions github-actions bot added this to the v0.101.0 milestone Nov 17, 2024
@fdncred fdncred added the deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way label Nov 19, 2024
@Bahex Bahex deleted the group-by-multiple-naming-1 branch March 22, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants