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

Fix/parser fails on comments #73

Merged
merged 5 commits into from
Nov 11, 2020
Merged

Conversation

jfcorbett
Copy link
Member

I ripped out everything to do with "fixing" missing column names, because it breaks support for comments placed to the right of tables, specifically on the same row as column names. Which basically violates the StarTable standard. As described in #72

This makes me a bit skeptical of the other aspects of ParseFixer... does any of these "auto fixes" break anything else? Will some of them not take users by surprise sometimes? Worth revisiting in the future and verifying that they do not cause more harm than good. @JanusWesenberg I'm sure you have an opinion here

@jfcorbett jfcorbett added the bug Something isn't working label Nov 10, 2020
@jfcorbett jfcorbett self-assigned this Nov 10, 2020
@JanusWesenberg
Copy link
Contributor

What is the use-case for this autofixer? According to the principle of least surprise, I would strongly vote for no autofixing and instead fail early on bad input: A key part of the value provided by starTable is the ability to review final inputs. If you cannot reliably predict how inputs will be handled, that becomes very hard.

@jfcorbett
Copy link
Member Author

jfcorbett commented Nov 11, 2020

TL;DR: I agree @JanusWesenberg . I turned this into Issue #71

The autofixer wasn't my idea or something I advocated for. I think the use cases were something along these lines:

  1. Ergonomics: Don't crash on the first tiny error, but instead collect all errors as they are encoutered, and crash at the end, reporting all errors at once. So if there are 10 errors, don't have to run the reader 10 times to reveal them all. <<< this is an option I think makes sense to have... but that's not what the default fixer does.
  2. Fix errors as they are encountered. <<< _I think this violates the principle of least surprise. The fixer second-guesses what the user "really" intended to write, and that's not something we should do by default. But ok if we give the user the option to do so, on their own terms. _
  3. Report all encountered errors, for user checking in an external tool, e.g. the StarTable Editor that @BennyLassen proposed. <<< This was actually demoed, and it's a good idea. But again, it should not be the default that we force-feed to all pdtable users. At best make it an option to be specified explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reading table blocks fails when there is a comment to the right of the column name row
3 participants