Resolve circular dependency #787

neelasha23 · 2023-08-09T06:21:37Z

Describe your changes

Moved the store utility functions like getting all saved snippets, finding dependents of key, etc to store_utils.py, and corresponding tests moved to test_store.py.
All util functions that access current connection moved from util.py to inspect.py, and corresponding tests moved to test_inspect.py
Renamed error_messages.py to error_handler.py and structured the code by adding more internal functions.
Moved logic of extracting table names from query to util.py.
Added functions to check if error is sqlalchemy error or not, and substring check in util.py. this is needed for the detailed error message logic.
Handling of DuckDB Parser error and pyodbc.ProgrammingError added. so some of the integration tests failing previously have now been fixed.

Issue number

Closes #749 #545

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--787.org.readthedocs.build/en/787/

neelasha23 · 2023-08-10T17:23:13Z

Please review @edublancas

edublancas · 2023-08-14T22:49:11Z

@neelasha23 I was about to review this but now I see that the tests are failing. what's the status? I guess there are merge conflicts due to recent changes in master?

neelasha23 · 2023-08-15T10:40:41Z

@neelasha23 I was about to review this but now I see that the tests are failing. what's the status? I guess there are merge conflicts due to recent changes in master?

Yes the snippets related tests started failing after I rebased. Fixed now. Please review.
@edublancas

neelasha23 · 2023-08-24T11:07:07Z

Please review @edublancas

edublancas

I just realized that I had reviewed this before but never submitted my observations (they still appeared as "pending" but I think that means you'll never saw them), so I'm submitting them now.

there's a lot of changes in this PR so please write a brief description so I know what to expect. e.g., "moved functions from a.py, b.py, moved functions from c.py into a separate module"

i guess this is mostly moving stuff around and not adding any new stuff so please explain

edublancas · 2023-08-15T15:46:41Z

CHANGELOG.md

@@ -12,6 +12,8 @@
 * [Fix] `ResultSet` footer only displayed when `feedback=2`
 * [Fix] Current connection and switching connections message only displayed when `feedback>=1`
 * [Fix] `--persist/--persist-replace` perform `ROLLBACK` automatically when needed
+* [Fix] Resolve circular dependency issue occurring due to multiple imports in `util.py`


these are internal changes, we don't put those in the changelog

edublancas · 2023-08-15T15:47:19Z

CHANGELOG.md

@@ -12,6 +12,8 @@
 * [Fix] `ResultSet` footer only displayed when `feedback=2`
 * [Fix] Current connection and switching connections message only displayed when `feedback>=1`
 * [Fix] `--persist/--persist-replace` perform `ROLLBACK` automatically when needed
+* [Fix] Resolve circular dependency issue occurring due to multiple imports in `util.py`
+* [Fix] Handling of DuckDB `Parser Error` and `pyodbc.ProgrammingError`


the changelog is intended for end-users, please rephrase it so it's more readable for them. as they likely don't know what a parser or programming error is

edublancas · 2023-08-15T15:49:40Z

src/sql/connection/connection.py

-                raise exceptions.UsageError(detailed_msg)
-            else:
-                print(e)
+            ErrorHandler(e).handle_exception()


I looked at the implementation and I don't see a need for this to be a class. lets turn this into a function (the function can internally call other functions to break the logic)

src/sql/store_utils.py

edublancas · 2023-08-15T15:52:56Z

src/tests/test_inspect.py

@@ -11,6 +11,13 @@
 from sql import inspect, connection


+EXPECTED_SUGGESTIONS_MESSAGE = "Did you mean :"


we have an extra space before the :

Suggested change

EXPECTED_SUGGESTIONS_MESSAGE = "Did you mean :"

EXPECTED_SUGGESTIONS_MESSAGE = "Did you mean:"

src/tests/test_inspect.py

src/tests/test_magic_cte.py

changelog fix moved store lint added fixture error handler typo test import fix import fix return fixed docstrings util test Minor import fix rebase added store_util test modified import

neelasha23 · 2023-08-26T17:40:37Z

Addressed your comments and updated the PR description @edublancas

edublancas · 2023-08-28T21:48:54Z

@neelasha23 good work!

when writing your initial comment, write each issue on a separate line:

closes #X
closes #Y

because if you do

closes #X #Y

github won't close both issues

neelasha23 marked this pull request as ready for review August 10, 2023 17:23

neelasha23 requested a review from edublancas as a code owner August 10, 2023 17:23

neelasha23 force-pushed the circ branch from e2d5205 to e0417da Compare August 14, 2023 15:11

neelasha23 force-pushed the circ branch from f08aa6e to 64baf14 Compare August 15, 2023 09:36

neelasha23 force-pushed the circ branch 5 times, most recently from 8cadc07 to 153ca93 Compare August 22, 2023 12:47

neelasha23 force-pushed the circ branch from 153ca93 to ce4481b Compare August 24, 2023 10:05

edublancas requested changes Aug 24, 2023

View reviewed changes

neelasha23 added 4 commits August 26, 2023 18:21

Resolve circular dependency

f7eb653

changelog fix moved store lint added fixture error handler typo test import fix import fix return fixed docstrings util test Minor import fix rebase added store_util test modified import

rebase

75244e4

review

0869c04

class removed

5872795

neelasha23 force-pushed the circ branch from f790be8 to 5872795 Compare August 26, 2023 12:51

neelasha23 added 2 commits August 26, 2023 20:55

minor

97af9be

test fix

b3fd194

neelasha23 requested a review from edublancas August 26, 2023 17:43

edublancas approved these changes Aug 28, 2023

View reviewed changes

edublancas merged commit 0e9d363 into ploomber:master Aug 28, 2023
23 checks passed

edublancas mentioned this pull request Aug 28, 2023

Circular dependency issue #545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve circular dependency #787

Resolve circular dependency #787

neelasha23 commented Aug 9, 2023 •

edited

Loading

neelasha23 commented Aug 10, 2023

edublancas commented Aug 14, 2023

neelasha23 commented Aug 15, 2023

neelasha23 commented Aug 24, 2023

edublancas left a comment

edublancas Aug 15, 2023

neelasha23 Aug 26, 2023

edublancas Aug 15, 2023

neelasha23 Aug 26, 2023

edublancas Aug 15, 2023

neelasha23 Aug 26, 2023

edublancas Aug 15, 2023

neelasha23 Aug 26, 2023

neelasha23 commented Aug 26, 2023

edublancas commented Aug 28, 2023

		@@ -11,6 +11,13 @@
		from sql import inspect, connection


		EXPECTED_SUGGESTIONS_MESSAGE = "Did you mean :"

Resolve circular dependency #787

Resolve circular dependency #787

Conversation

neelasha23 commented Aug 9, 2023 • edited Loading

Describe your changes

Issue number

Checklist before requesting a review

neelasha23 commented Aug 10, 2023

edublancas commented Aug 14, 2023

neelasha23 commented Aug 15, 2023

neelasha23 commented Aug 24, 2023

edublancas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neelasha23 commented Aug 26, 2023

edublancas commented Aug 28, 2023

neelasha23 commented Aug 9, 2023 •

edited

Loading