8000 RFC for table interpretation by felix-oq · Pull Request #128 · jvalue/jayvee · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

RFC for table interpretation #128

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

Merged
merged 3 commits into from
Feb 14, 2023
Merged

RFC for table interpretation #128

merged 3 commits into from
Feb 14, 2023

Conversation

felix-oq
Copy link
Contributor
@felix-oq felix-oq commented Feb 7, 2023

Part of #85

@felix-oq felix-oq requested review from rhazn and georg-schwarz and removed request for rhazn February 7, 2023 09:37
Copy link
Contributor
@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

I see I was removed but I like the concept + the RFC presentation is great and clear, amazing.

The only thing I am unsure about is (as you already point out) the double semantics of "col1" typed text with and without header is weird but I also have no really better idea right now. So I'd go for this and we can revisit it later.

Comment on lines +47 to +60
block MyTableInterpreter oftype TableInterpreter {
header: false;
columns: [
"col1" typed text,
"col2" typed integer,
"col3" typed boolean,
"col4" typed decimal,
]
}
```

The attribute `header: false` indicates that the sheet has no topmost header row, so the column names are taken from the
provided names in the `columns` attribute. There, each entry describes a column by defining its name and its type. Note
that the order of columns matters, as the first entry maps to the first column `A`, the second to `B` and so on.
Copy link
Member

Choose a reason for hiding this comment

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

Nice! My only concern is the semantic "the order matters" in an array syntax []. That might be counter-intuitive for programmers, we could offer some clarification elsewhere, though, e.g. in the VSCode plugin adding a gray "A" in front or so.

@felix-oq felix-oq changed the title DRAFT: RFC for table interpretation DISCUSSION: RFC for table interpretation Feb 7, 2023
@felix-oq
Copy link
Contributor Author
felix-oq commented Feb 7, 2023

@rhazn @georg-schwarz thanks for the feedback! Please let me know whether we need a further discussion or you consider this accepted.


Additionally, I'd like to share my thoughts regarding semantic ordering differences:

  • When a header is present, the column names in columns denote both the name and the location of the column.
  • When no header is present, the column names in columns only denote the name. So the location of the column is specified implicitly via the ordering.

To resolve the difference, we could

  • enable the explicit specification of column locations in columns, e.g. via "col1" typed text at column A statements
  • or provide the ability to enrich the semantic meaning of column names with the column location (if no header is present), e.g. allow a separate mapping of column names and column locations like column A called "col1".

Then the ordering in columns would always be arbitrary and would not have any semantic meaning. The downside would be a harder to read syntax with possibly more boilerplate code.

@rhazn
Copy link
Contributor
rhazn commented Feb 7, 2023

Imho having ordering matter in the array itself is not an issue. I think it would be nice to separate naming columns from using the names. This way the typing semantics would always be the same (name and type).

Then you could get the names by either namesFromHeader: true or columnNames: ["ordered", "list", "of", "names] and throw an error if a block has both namesFromHeader and columnNames. That would make attributes easier to read and the semantics consistent.

Either way, if you go with your choice here or my suggestion, I'd consider both fine.

8000
@georg-schwarz
Copy link
Member

I'm fine with either way, also with the original if we document it accordingly.

@felix-oq
Copy link
Contributor Author
felix-oq commented Feb 7, 2023

I really like the proposal from @georg-schwarz with having IDE hints that display the corresponding column. I researched a bit and found the so called inlay hints. See a VS Code demo video and the corresponding LSP specification. Not sure though how easily that can be implemented using Langium.

Edit: Langium has no native support currently but it is planned to provide an interface for them, see eclipse-langium/langium#595.

@rhazn
Copy link
Contributor
rhazn commented Feb 7, 2023

I'd caution against that. It is cool but we are fundamentally developing a language, not an IDE. So if the language is unclear/hard to understand without VSCode I think we are making a mistake. I am aware that implementing it with the LSP means the feature is not bound to VSCode but it is bound to the user installing a specific tool to deal with .jv files.

@felix-oq
Copy link
Contributor Author

Then I propose to go with the table building concept as stated here in the RFC, without further changes. Of course we can revisit the approach and improve it in the future.

@felix-oq felix-oq changed the title DISCUSSION: RFC for table interpretation RFC for table interpretation Feb 14, 2023
@rhazn
Copy link
Contributor
rhazn commented Feb 14, 2023

👍

@felix-oq felix-oq merged commit a8f628d into main Feb 14, 2023
@felix-oq felix-oq deleted the rfc-table-interpretation branch February 14, 2023 09:02
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0