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

allow leading comments in SQL input field #653

Merged
merged 2 commits into from Feb 5, 2020

Conversation

jaywgraves
Copy link
Contributor

@jaywgraves jaywgraves commented Dec 21, 2019

this changes the SQL validation to allow for lines that are commented out

my main use case for this is that I like to write a succession of queries when trying to solve a problem.
In most native SQL clients there is a key binding that will run just the current highlighted query or the program is smart enough to run just the query that the cursor is in if it's properly delimited with a ';'.
Typically my workflow will start with a single simple query and I'll copy/paste it to a new query below when I want to make big changes while debugging. This makes it easy to go back to a working version above when the query doesn't work.
Since datasette sends the whole query to the DB I have to comment out the older queries by prefixing each line with --. This gets caught by the validators when I use my typical strategy of copy/pasting each successive query below the last one.
so this is just a simple fix to allow for a query to be sent to the DB with leading comments.

@simonw
Copy link
Owner

simonw commented Feb 4, 2020

I think there's one nasty edge-case here that we need to worry about: SQLite allows newlines inside of single quoted strings and I've actually started using that in quite a few places - it's great for embedding markdown in a string for example.

The way you're stripping comments right now splits on newlines and removes any lines that start with --. I believe that will mangle the following example:

select '# Hello there

* This is a list
* of items
--
[And a link](https://github.com/simonw/datasette-render-markdown).'
as demo_markdown

@simonw
Copy link
Owner

simonw commented Feb 4, 2020

This is the kind of problem that has made me think that Datasette would really benefit from including a smart SQLite-syntax SQL parser.

Writing one is a bit of a challenge though! There's an example (derived from SQLite SELECT statements) included in pyparsing here but I've not spent much time evaluating it: https://github.com/pyparsing/pyparsing/blob/master/examples/select_parser.py

@simonw
Copy link
Owner

simonw commented Feb 4, 2020

We can probably solve this without a SQL parser though. Really all we care about here is that if the FIRST lines of the statement begin with -- we ignore them and only validate the statement starting from the first non-commented line. I think we can do that without single quoted strings causing us confusion.

@jaywgraves
Copy link
Contributor Author

jaywgraves commented Feb 4, 2020

I think the existing code will be OK even if I strip the lines in the middle of a new line delimited string.

It's only used for the validation, SQLite handles the -- just fine and the whole SQL textarea still gets sent once it passes validation.

I can add your test case to my branch later this evening though.

@jaywgraves
Copy link
Contributor Author

jaywgraves commented Feb 4, 2020

but this also doesn't have to land at all if it doesn't match your use case.

@simonw
Copy link
Owner

simonw commented Feb 4, 2020

You may well be right there! Let's add a test that demonstrates it.

@simonw
Copy link
Owner

simonw commented Feb 4, 2020

Looks like SQLite supports /* ... */ style comments as well. I don't think supporting those should be a requirement to land this though. https://www.sqlite.org/lang_comment.html

@simonw simonw merged commit 33a12c8 into simonw:master Feb 5, 2020
1 check passed
simonw added a commit that referenced this pull request Feb 5, 2020
@simonw
Copy link
Owner

simonw commented Feb 5, 2020

This is shipped in Datasette 0.35. Here's a demo of it working:

https://latest.datasette.io/fixtures?sql=--+this+is+a+comment%0D%0Aselect+*+from+%5B123_starts_with_digits%5D

Compare with https://v0-34.datasette.io/fixtures?sql=--+this+is+a+comment%0D%0Aselect+*+from+%5B123_starts_with_digits%5D which returned an error.

@jaywgraves jaywgraves deleted the jg/allow_leading_comments branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants