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

Add parametrized queries #137

Merged

Conversation

tonykploomber
Copy link

@tonykploomber tonykploomber commented Feb 17, 2023

Describe your changes

1. Disable the default IPython var_expand for {a}, $a case

We disable the default variable substitution by adding @no_var_expand to the line_magic, since we still need to support the default variable substitution + new parametrized fashion in recent versions, we move that parsing part in the SQLCommand class

2. Add _var_expand in SQLCommand class

The logic for supporting {a}, $a (IPython) + {{a}} (new param feature) variable substitution is:

  1. Handle the line & cell original string by jinja2 (new param feature)
  2. Handle the line & cell original string by IPython var_expand native function to support the IPython way

This execution order matters, since {a} is the subset of {{a}}, we need to handle the {{a}} case before being handled by {a}

P.S. After those string are parsed, the :a part will still be kept, since it's handled by SQLAlchemy engine

Conclusion

This PR fulfill three requirements:

  1. Add @no_var_expand to the execute line magic. We handle the var_expand by ourself
  2. When there is variable substitution in {a}, $a, :a. We display message (can be discussed)
Please aware the variable substition as {a}, $a, and :a format has been deprecated.
Use {{a}} instead.
  1. Support variable substitution in {{a}} fashion, the values will fetch from user name space now

Preview

:a - with deprecated message

Screenshot 2023-02-22 at 11 21 54 PM

{a} - with deprecated message

Screenshot 2023-02-22 at 11 28 20 PM

$a - with deprecated message

Screenshot 2023-02-22 at 11 29 19 PM

{{a}} - new parameter - consumed value from existing scope

Screenshot 2023-02-22 at 11 30 28 PM

Follow up

We need to support the arg --param & --use-global with {{a}} format in next PR

Issue ticket number and link

Closes #93

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added thorough tests (when necessary).
  • I have added the right documentation in the docstring and changelog (when needed)

📚 Documentation preview 📚: https://jupysql--137.org.readthedocs.build/en/137/

@tonykploomber tonykploomber linked an issue Feb 17, 2023 that may be closed by this pull request
@tonykploomber tonykploomber marked this pull request as draft February 17, 2023 20:45
@tonykploomber tonykploomber marked this pull request as ready for review February 23, 2023 04:36
src/sql/command.py Outdated Show resolved Hide resolved
src/sql/command.py Show resolved Hide resolved
src/sql/magic.py Outdated Show resolved Hide resolved
src/sql/command.py Outdated Show resolved Hide resolved
"Please aware the variable substition"
" as {a}, $a, and :a format has been deprecated."
)
print("Use {{a}} instead.")

Choose a reason for hiding this comment

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

also, let's add a tutorial in our docs describing this feature, and include the link to it

e.g., https://jupysql.ploomber.io/en/latest/user-guide/templating.html

https://github.com/ploomber/contributing/blob/main/documentation/notebooks.md

also in the current documentation where we explain the variable substitution, let's add a deprecation notice

```{versionchanged} 0.5.7
This has been deprecated use '{{a}}' [and add link to the new tutorial
```

Choose a reason for hiding this comment

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

ah and one more thing: let's add a new document in the user guide where we list the incompatibilities with the ipython-sql project. just 1-2 sentences saying we use {{a}} instead of the old method and a link to the tutorial

Copy link
Author

Choose a reason for hiding this comment

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

New template page in user-guide: https://jupysql--137.org.readthedocs.build/en/137/user-guide/template.html
Add deprecation notice in intro: https://jupysql--137.org.readthedocs.build/en/137/intro.html#variable-substitution

incompatibilities:
Screenshot 2023-02-28 at 3 01 47 PM

Please ignore the latest build fail, since the new page user-guide/template.html haven't deployed to jupysql.ploomber.io, it won't pass the broken-links action in our CI

@idomic
Copy link

idomic commented Feb 28, 2023

Looks like the CI is failing on broken links

@tonykploomber
Copy link
Author

Looks like the CI is failing on broken links

Yes. this is expected, #137 (comment)

Since we added the new page user-guide/template.md, and also referred it in another page (vs.md), but the template page haven't deployed to jupysql.ploomber.io, so it failed on broken-link

The second newest commit 6c4d9cb is a success build, since it it's without referring the template page. The latest commit is with that

@tonykploomber
Copy link
Author

The see more link wouldn't pass the broken-link, revert for now

@edublancas
Copy link

looks like the docs are failing, my guess is that this is due to the new jupyter book version: https://github.com/executablebooks/jupyter-book/releases

let's pin to <0.14.0 and see if that fixes it

@tonykploomber tonykploomber changed the title 93 - adding parametrized queries Add parametrized queries Mar 1, 2023
@tonykploomber
Copy link
Author

@edublancas
Sync with latest master, you can review again!

doc/intro.md Show resolved Hide resolved
src/sql/command.py Show resolved Hide resolved
src/sql/command.py Outdated Show resolved Hide resolved
src/sql/command.py Outdated Show resolved Hide resolved
src/sql/magic.py Outdated Show resolved Hide resolved
src/sql/command.py Show resolved Hide resolved
@edublancas edublancas merged commit 6c31651 into ploomber:master Mar 3, 2023
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.

adding parametrized queries
3 participants