CTE deprecation #486

neelasha23 · 2023-05-12T12:44:46Z

Describe your changes

Deprecate --with when forming CTEs

Issue number

Closes #166

Checklist before requesting a review

Performed a self-review of my code
Formatted my code with pkgmt format
Added tests (when necessary).
Added docstring documentation and update the changelog (when needed)

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

neelasha23 · 2023-05-23T15:14:28Z

Covered most of the test cases I could think of. Still testing to check for bugs. Meanwhile, one round of review would be helpful.

@edublancas @idomic

Also, there is a circular dependency issue whenever util.py is imported in some of the modules. So had to create separate module, e.g., query_util.py for now.

ImportError while loading conftest '/Users/neelashasen/Dev/jupysql/src/tests/conftest.py'.
src/tests/conftest.py:8: in <module>
    from sql.magic import SqlMagic, RenderMagic
../../miniforge3/envs/jupysql/lib/python3.10/site-packages/sql/__init__.py:1: in <module>
    from .magic import RenderMagic, SqlMagic, load_ipython_extension
../../miniforge3/envs/jupysql/lib/python3.10/site-packages/sql/magic.py:22: in <module>
    import sql.connection
../../miniforge3/envs/jupysql/lib/python3.10/site-packages/sql/connection.py:11: in <module>
    from sql.store import store
../../miniforge3/envs/jupysql/lib/python3.10/site-packages/sql/store.py:12: in <module>
    from sql import util
../../miniforge3/envs/jupysql/lib/python3.10/site-packages/sql/util.py:1: in <module>
    from sql import inspect
../../miniforge3/envs/jupysql/lib/python3.10/site-packages/sql/inspect.py:4: in <module>
    from sql.connection import Connection
E   ImportError: cannot import name 'Connection' from partially initialized module 'sql.connection' (most likely due to a circular import) (/Users/neelashasen/miniforge3/envs/jupysql/lib/python3.10/site-packages/sql/connection.py)

yafimvo

Added some comments

General notes

I think we can test the util functions as well (e.g. get_key_dependents, del_saved_key, del_all_saved_keys)
There is missing documentation for some of the functions

doc/api/magic-snippets.md

src/sql/magic_cmd.py

src/tests/test_magic_cmd.py

src/sql/util.py

src/tests/test_magic_plot.py

src/tests/test_magic_cmd.py

CHANGELOG.md

doc/compose.md

src/sql/__init__.py

src/sql/command.py

src/sql/magic.py

src/sql/magic_cmd.py

src/sql/magic_plot.py

src/sql/store.py

src/tests/conftest.py

src/tests/test_compose.py

tonykploomber · 2023-05-27T09:32:52Z

By the way, do we need to take telemetry into this?

neelasha23 · 2023-05-29T14:58:30Z

Added some comments

General notes

I think we can test the util functions as well (e.g. get_key_dependents, del_saved_key, del_all_saved_keys)

There is missing documentation for some of the functions

Added unit tests and docstrings

edublancas

minor comments

src/sql/magic.py

src/sql/magic_cmd.py

edublancas · 2023-05-30T03:31:47Z

there's no need to add telemetry because this isn't adding any new options (it's automatically executed whenever a SQL snippet runs)

neelasha23 · 2023-05-30T13:24:11Z

Fixed all the review comments.
@edublancas @yafimvo

tonykploomber

LG

yafimvo

added minor notes. besides that lgtm.

src/tests/test_magic_cmd.py

doc/api/magic-snippets.md

doc/compose.md

src/sql/magic.py

src/sql/sqlcmd.py

src/sql/util.py

src/tests/test_extract_tables.py

edublancas

just rebase, update the changelog entry and fix merge conflicts

CHANGELOG.md

CTE depr Warning msg, sqlglot Lint tests warn test conditions for extract Empty commit removed with test exception handling Fixed tests Lint typo error test condition for typo suggestions plot changes merge conflict fix test Fix test fix test tests list of with sqlcmd delete sqlcmd tests rebase Fixes fixed test fixes removed test non-existent tbl Test fix fix typo test fix typo test space removed delete snippet test test fix Docs docs toc changelog modified args modified versionchanged added message changed tests sqlcmd added deprecation warning changed tests depr warning removed if exists debug prints added clean_conns rebase test fix rebase issues revert error msg test for snippet utils telemetry removed Minor changes Moved string refactor plot tests plot_typo Renamed test file Removed line test fix test fix Review comments added issue numbers rebase lint

neelasha23 changed the title ~~CTE deprecation~~ CTE deprecation - WIP May 12, 2023

neelasha23 marked this pull request as draft May 12, 2023 12:45

neelasha23 force-pushed the cted branch 2 times, most recently from c2bfe3e to 7331258 Compare May 22, 2023 10:31

neelasha23 force-pushed the cted branch from 86214ca to b103039 Compare May 23, 2023 11:47

neelasha23 marked this pull request as ready for review May 23, 2023 15:14

neelasha23 requested review from yafimvo, edublancas and tonykploomber May 23, 2023 15:19

neelasha23 changed the title ~~CTE deprecation - WIP~~ CTE deprecation May 24, 2023

yafimvo requested changes May 24, 2023

View reviewed changes

edublancas requested changes May 24, 2023

View reviewed changes

neelasha23 force-pushed the cted branch from cc3b71f to ffddee4 Compare May 25, 2023 06:33

mehtamohit013 mentioned this pull request May 25, 2023

%sqlplot bar and pie charts #508

Merged

4 tasks

tonykploomber closed this May 27, 2023

tonykploomber reopened this May 27, 2023

neelasha23 force-pushed the cted branch from efbf719 to 640356b Compare May 29, 2023 05:55

edublancas requested changes May 29, 2023

View reviewed changes

src/sql/magic.py Show resolved Hide resolved

src/sql/magic_cmd.py Show resolved Hide resolved

neelasha23 requested review from yafimvo and edublancas May 30, 2023 13:24

tonykploomber previously approved these changes May 30, 2023

View reviewed changes

neelasha23 dismissed tonykploomber’s stale review via e5f0665 May 31, 2023 05:44

neelasha23 force-pushed the cted branch 2 times, most recently from 1477903 to ab4f1fc Compare May 31, 2023 06:15

yafimvo requested changes May 31, 2023

View reviewed changes

neelasha23 force-pushed the cted branch 2 times, most recently from 20e2420 to 5a3cb4e Compare May 31, 2023 14:34

edublancas previously approved these changes Jun 1, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

neelasha23 dismissed edublancas’s stale review via f64f646 June 1, 2023 05:45

neelasha23 force-pushed the cted branch from 5a3cb4e to f64f646 Compare June 1, 2023 05:45

neelasha23 force-pushed the cted branch from f64f646 to 2552066 Compare June 1, 2023 05:59

neelasha23 added 2 commits June 1, 2023 11:43

added to bar, pie"

c685d3f

Empty commit

f0120d3

neelasha23 requested a review from edublancas June 1, 2023 11:46

edublancas approved these changes Jun 1, 2023

View reviewed changes

edublancas merged commit 6ac6f73 into ploomber:master Jun 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CTE deprecation #486

CTE deprecation #486

neelasha23 commented May 12, 2023 •

edited

Loading

neelasha23 commented May 23, 2023

yafimvo left a comment

tonykploomber commented May 27, 2023

neelasha23 commented May 29, 2023

edublancas left a comment

edublancas commented May 30, 2023

neelasha23 commented May 30, 2023

tonykploomber left a comment

yafimvo left a comment

edublancas left a comment

CTE deprecation #486

CTE deprecation #486

Conversation

neelasha23 commented May 12, 2023 • edited Loading

Describe your changes

Issue number

Checklist before requesting a review

neelasha23 commented May 23, 2023

yafimvo left a comment

Choose a reason for hiding this comment

tonykploomber commented May 27, 2023

neelasha23 commented May 29, 2023

edublancas left a comment

Choose a reason for hiding this comment

edublancas commented May 30, 2023

neelasha23 commented May 30, 2023

tonykploomber left a comment

Choose a reason for hiding this comment

yafimvo left a comment

Choose a reason for hiding this comment

edublancas left a comment

Choose a reason for hiding this comment

neelasha23 commented May 12, 2023 •

edited

Loading