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

Remove need for --begin-sql comments #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

akdor1154
Copy link

@akdor1154 akdor1154 commented Mar 13, 2021

I've been stuffing around with this for ages, and I finally think I've managed to remove the need for --begin-sql and --end-sql or ; delimiters for the syntax block.

The strategy is to detect "select", "with", or "--", and just mark from there to the end of the string as SQL. This fits my usage perfectly.. unsure if it is useful for everyone (e.g. you might want to add "INSERT", etc, or you might not like this approach at all..)

I've updated demo.py|png to show this in action, along with the readme:
demo

Additionally I've set the name of the matched section to meta.embedded.SQL to set the language mode to SQL - this fixes some final highlight stuff, and makes VSCode's bracket matching and comment/uncomment features work properly as well. You might want to pinch this even if you don't like the rest of this PR.

One drawback is that I can't force the string to start with select|with|--; it has to just find it at any point in the string (due to the single-line-context-only property of the highlighting system). So if you have something like

str = """ I'd like to go and select some nice flowers """,
                             ^^^ sql from here

it will "mis-fire". I'm not aware of any way to prevent this.

Cheers
Jarrad

@alanwilter
Copy link

alanwilter commented Apr 19, 2021

And what about adding support for raw string? I have queries where I need to add r"""select ... or even rf"""" select {foo}... because those queries has things like:

select regexp_split_to_table(path::text, '\.')::int as ancestor from ...

If I don't add r""" (for raw), flake8 will complain about invalid escape sequence '\.' flake8(W605).

@@ -5,19 +5,17 @@
"injectionSelector": "L:string.quoted.multi.python, L:meta.fstring.python - (comment.line.number-sign.python, punctuation.definition.comment.python)",
"patterns": [
{
"begin": "( *--sql| *--beginsql| *--begin-sql)",
Copy link

@cikay cikay Nov 16, 2021

Choose a reason for hiding this comment

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

I do not recommend to remove the current identifiers. It would break all implementation. Add new ones but do not remove the current ones.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, thanks for getting to this :)

Copy link
Author

Choose a reason for hiding this comment

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

Ah, you are not maintainer haha. You're probably right but ill wait to hear from repo owner/member if they are still alive...

@munro
Copy link

munro commented Dec 13, 2021

@akdor1154 I've created a fork that adds support to this, it's still a little buggy, but I really want this to work so I can give y'all access so we can start hacking on a solution—so far it's been "good enough" for me to not spend any more time on this though. :) You can install better-python-string-sql in the VS Code market place!

https://marketplace.visualstudio.com/items?itemName=Submersible.better-python-string-sql

https://github.com/Submersible/better-python-string-sql/

Also, I imagine eventually this all will be rolled up into the MagicPython extension, as they're already trying to get support for SQL there: MagicStack/MagicPython#27

What I need help on is setting up some automated tests, so I can start keeping track of bugs I encounter while using it, so it can be perfect 💫

@akdor1154
Copy link
Author

Ah nice! I can't promise I'll look into testing in the immediate future, I don't know anything about testing syntaxes. It sounds difficult to DIY unless MS have published tooling for this. I did find https://github.com/PanAeon/vscode-tmgrammar-test with a quick google, if I get spare time (hahahahahahaha) I'll play with how that works with your repo.

Will definitely start using your fork, thanks.

@ptweir
Copy link
Owner

ptweir commented Nov 6, 2023

hey all, sorry I haven't been very diligent about maintaining this project. I worry about breaking backwards compatibility, so will hold off merging this for now. Get in touch if I'm misreading though!

@peteromano
Copy link

peteromano commented Nov 10, 2023

hey all, sorry I haven't been very diligent about maintaining this project. I worry about breaking backwards compatibility, so will hold off merging this for now. Get in touch if I'm misreading though!

Just started using this 5 minutes ago; this is awesome!

A huge help would be to remove semicolon as terminator, or make it configurable.

This is pretty much just laziness, but in my migration files I'll do multi sql statements:

op.execute('''

--sql
create table "x" (
    "id" integer not null
);

--sql
CREATE UNIQUE INDEX x_pkey ON x USING btree (id);

--sql
alter table "x" add constraint "x_pkey" PRIMARY KEY using index "x_pkey";

''')

Would be cool to not have to add --sql before each statement (or not have to use multiple op.execute's)

This would be totally fine, imo:

op.execute('''--begin-sql

create table "x" (
  "id" integer not null
);

CREATE UNIQUE INDEX x_pkey ON x USING btree (id);

alter table "x" add constraint "x_pkey" PRIMARY KEY using index "x_pkey";

--end-sql''')

Also, the semicolon hightlighted in red makes it look like a sql syntax issue, too :)

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.

6 participants