Skip to content

allow tables to have annotations#9613

Merged
sholderbach merged 6 commits intonushell:mainfrom
1Kinoti:table
Jul 7, 2023
Merged

allow tables to have annotations#9613
sholderbach merged 6 commits intonushell:mainfrom
1Kinoti:table

Conversation

@1Kinoti
Copy link
Copy Markdown
Contributor

@1Kinoti 1Kinoti commented Jul 5, 2023

Description

follow up to #8529 and #8914

this works very similarly to record annotations, only difference being that

table<name: string>
      ^^^^  ^^^^^^
      |     | 
      |     represents the type of the items in that column
      |
      represents the column name

more info on the syntax can be found here

User-Facing Changes

[BREAKING CHANGE]
this change adds a field to SyntaxShape::Table so any plugins that used it will have to update and include the field. though if you are unsure of the type the table expects, SyntaxShape::Table(vec![]) will suffice

@sholderbach sholderbach added A:type-system Problems or features related to nushell's type system notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes notes:mention Include the release notes summary in the "Hall of Fame" section labels Jul 5, 2023
@1Kinoti 1Kinoti marked this pull request as draft July 5, 2023 19:56
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jul 5, 2023

This is pretty cool

ls | describe
table<name: string, type: string, size: filesize, modified: date> (stream)

we still need to get our story straight in the codebase whether it's a date or a datetime. that needs to be fixed at some point.

@1Kinoti 1Kinoti marked this pull request as ready for review July 6, 2023 06:50
@1Kinoti
Copy link
Copy Markdown
Contributor Author

1Kinoti commented Jul 6, 2023

we still need to get our story straight in the codebase whether it's a date or a datetime. that needs to be fixed at some point.

i think date is just fine, although changing from datetime will be a breaking change. Type displays it as 'date' while SyntaxShape displays it as 'datetime'

amtoine
amtoine reviewed Jul 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not understanding all of the parsing changes, but it looks ok to me and the rest is great: the new vec![] in Table and the tests 👌

i'll approve this and let parser-experts have a look to the parser changes 😌

thanks @1Kinoti, as always great job 💪

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jul 6, 2023

i think date is just fine, although changing from datetime will be a breaking change. Type displays it as 'date' while SyntaxShape displays it as 'datetime'

@1Kinoti This is what I'm saying. We're inconsistent in our usage of date/datetime. Some places it's date. Other places it's datetime. I'm just saying we need to be consistent with whatever we use. Having said that, sorry for bringing up this discussion here since it has nothing to do with your PR.

sholderbach
sholderbach approved these changes Jul 6, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my cursory reading of the parser nothing stands out to me, the implementation of SyntaxShape/Type looks good, and its great how you cover the happy and necessary unhappy paths.
As no existing tests had to change, I think we should move ahead and getting it into play testing!

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jul 6, 2023

oops, looks like there's a conflict now. :(

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Jul 7, 2023

agree with @sholderbach 👌

@1Kinoti, the conflict has been very likely introduced by #9404: the update_cells.rs module has moved from the nu-command crate to nu-cmd-extra => should be just a matter of copy-pasting the changes there 😌

@1Kinoti 1Kinoti marked this pull request as draft July 7, 2023 08:25
@1Kinoti 1Kinoti marked this pull request as ready for review July 7, 2023 08:30
@1Kinoti
Copy link
Copy Markdown
Contributor Author

1Kinoti commented Jul 7, 2023

@amtoine since the review is done, i just rebased. hope this is okay :)

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Jul 7, 2023 •
edited
Loading

@amtoine since the review is done, i just rebased. hope this is okay :)

mm, personally i dunno 🤔

from a reviewer's point of view, it broke the review as all the hashes changed, there is no merge commit and GitHub does not show the merge changes, rather everything that happened on the main branch in the meantime 😭

again, these are MY two cents, but i think rebasing once a review has started will allways break the review process at some point in the PR branch 😕

@1Kinoti
Copy link
Copy Markdown
Contributor Author

1Kinoti commented Jul 7, 2023

since the review has already been done, i think it's mostly okay, right?

@sholderbach
Copy link
Copy Markdown
Member

sholderbach commented Jul 7, 2023

I am fine with it!

@sholderbach sholderbach merged commit 8e38596 into nushell:main Jul 7, 2023
@1Kinoti 1Kinoti mentioned this pull request Jul 7, 2023
Merged
@1Kinoti 1Kinoti deleted the table branch July 8, 2023 15:51
@fdncred fdncred mentioned this pull request Jul 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amtoine amtoine amtoine left review comments

@sholderbach sholderbach sholderbach approved these changes

Assignees

No one assigned

Labels

A:type-system Problems or features related to nushell's type system notes:breaking-changes This PR implies a change affecting users and has to be noted in the release notes notes:mention Include the release notes summary in the "Hall of Fame" section

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@1Kinoti @fdncred @amtoine @sholderbach