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

Move feat from %sqlrender to %sqlcmd snippets #656

Closed
wants to merge 4 commits into from
Closed

Move feat from %sqlrender to %sqlcmd snippets #656

wants to merge 4 commits into from

Conversation

bbeat2782
Copy link

@bbeat2782 bbeat2782 commented Jun 23, 2023

Describe your changes

Moved %sqlrender feature to %sqlcmd snippets, added FutureWarning when user call %sqlrender, modified API Reference documentation, and modified test functions to test %sqlcmd snippets {snippet_name}

Issue number

Closes #647

Checklist before requesting a review


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

Moved features, added FutureWarning, update API Reference documentation, modified test functions
src/sql/sqlcmd.py Show resolved Hide resolved
Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

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

@bbeat2782
lgtm. Please rebase.

@edublancas do you think we should remove the sqlrender guide from the doc tree?

image

`-d`/`--delete` Delete a snippet.

`-D`/`--delete-force` Force delete a snippet. This may be useful if there are other dependent snippets, and you still need to delete this snippet.

`-A`/`--delete-force-all` Force delete a snippet and all dependent snippets.

```{code-cell} ipython3
cte = %sqlcmd snippets gentoo
print(cte)
Copy link

@neelasha23 neelasha23 Jun 26, 2023

Choose a reason for hiding this comment

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

This is currently displaying

WITH
SELECT * FROM penguins.csv where species == 'Gentoo'

Is it supposed to display WITH even when it is not used as subquery as part of another query?

Also, previously we could sqlrender the final query as well, that uses a snippet as subquery. e.g.
SELECT * from gentoo where bill_length < 50... can you add a similar example ?

Copy link
Author

@bbeat2782 bbeat2782 Jun 26, 2023

Choose a reason for hiding this comment

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

Also, previously we could sqlrender the final query as well, that uses a snippet as subquery. e.g.
SELECT * from gentoo where bill_length < 50... can you add a similar example ?

For this one, when you mentioned sqlrender showing the final query, are you referring to something like the following?
Screen Shot 2023-06-26 at 1 20 06 PM

Choose a reason for hiding this comment

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

@neelasha23: yes, this is a bug we discovered recently. I reported it on #657, @bbeat2782 will work on it.

@bbeat2782: please work on this first, and then tackle the other one

@neelasha23
Copy link

Added some comments. Few other observations:

  1. There are places in the docs where we use sqlrender,e.g., cte. I think we should replace those as well.
  2. Please resolve merge conflicts

@bbeat2782
Copy link
Author

1. There are places in the docs where we use sqlrender,e.g., cte. I think we should replace those as well.

I was thinking about replacing %sqlrender with %sqlcmd snippets with a separate PR, but if you think it works better with one PR, I will add those changes to this PR.

@edublancas
Copy link

I was thinking about replacing %sqlrender with %sqlcmd snippets with a separate PR, but if you think it works better with one PR, I will add those changes to this PR.

yes, please tackle documentation changes here. usually whenever we modify a feature, the PR that changes the feature also updates the docs.

Copy link

@edublancas edublancas left a comment

Choose a reason for hiding this comment

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

please address the observations from @neelasha23

@edublancas
Copy link

@bbeat2782: we just merged #654 which moved the implementation of %sqlcmd to separate files (same logic, just re-organized in different files). this is causing merge conflicts. check if you can fix them. otherwise it might be better to branch from master and apply your changes manually to the new files

@bbeat2782 bbeat2782 self-assigned this Jun 27, 2023
@bbeat2782
Copy link
Author

bbeat2782 commented Jun 27, 2023

@edublancas it says src/sql/sqlcmd.py is the conflicting file but the Resolve conflicts button is disabled from my side with a message saying "Only those with write access to this repository can merge pull requests."

So it seems like I would have to apply the changes I made manually.

@bbeat2782 bbeat2782 removed their assignment Jun 27, 2023
@edublancas
Copy link

@edublancas it says src/sql/sqlcmd.py is the conflicting file but the Resolve conflicts button is disabled from my side with a message saying "Only those with write access to this repository can merge pull requests."

I think you'll be able to do it from the CLI, but yeah feel free to do it manually (create a new branch from master and then apply your changes)

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.

move feature from %sqlrender -> %sqlcmd snippets
4 participants