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

snippets display improvement #716

Merged
merged 1 commit into from
Jul 11, 2023
Merged

snippets display improvement #716

merged 1 commit into from
Jul 11, 2023

Conversation

bbeat2782
Copy link

@bbeat2782 bbeat2782 commented Jul 6, 2023

Describe your changes

Change %sqlcmd snippets output into displaying 1. snippet name 2. snippet query 3. snippet table.

Still working on 1. figuring out how to retrieve SqlMagic.displaylimit 2. adding test functions 3. update documentation if needed, but I want to know whether it's on the right direction or not.

Issue number

Closes #648

Checklist before requesting a review


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

@bbeat2782
Copy link
Author

bbeat2782 commented Jul 6, 2023

@edublancas

This is the current output of %sqlcmd snippets. Am I on the right track?

Screen Shot 2023-07-06 at 1 14 45 PM

@edublancas
Copy link

@bbeat2782 you don't need to query the tables. instead, you can create a table that displays the snippet names

e.g.

----------------
| some snippet |
----------------
| another           |
----------------

you can use this:

class Table:

@bbeat2782
Copy link
Author

@bbeat2782 you don't need to query the tables. instead, you can create a table that displays the snippet names

e.g.

----------------
| some snippet |
----------------
| another           |
----------------

you can use this:

class Table:

@edublancas Oh that is what you meant when you said table. Thanks. Will make the change.

@bbeat2782
Copy link
Author

Output of %sqlcmd snippets when no snippet is stored.
Screen Shot 2023-07-07 at 10 10 08 AM

Output of %sqlcmd snippets when chinstrap and chinstrap_sub are stored.
Screen Shot 2023-07-07 at 10 10 11 AM

The above images are screen captured images from snippet_display_improvement.ipynb.zip

@bbeat2782 bbeat2782 marked this pull request as ready for review July 7, 2023 18:01
src/tests/test_magic_cmd.py Outdated Show resolved Hide resolved
src/tests/test_magic_cmd.py Outdated Show resolved Hide resolved
@bbeat2782 bbeat2782 changed the title snippets draft update snippets display improvement Jul 9, 2023
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.

looks good. Added one comment about the test.

),
),
(
"%sqlcmd snippets -d high_price_a",
Copy link

Choose a reason for hiding this comment

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

Deleting a snippet might affect some other tests. To make sure we are not, I think you can create a new snippet and see if it's in the table. then, clean up these snippets. I would test the following cases: no snippets, 1, 2, and 3 snippets.

Copy link
Author

Choose a reason for hiding this comment

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

@yafimvo
To ensure it doesn't affect other tests, I created a new @pytest.fixture for this test function for creating and deleting snippets.

@bbeat2782 bbeat2782 requested a review from yafimvo July 10, 2023 21:15
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.

lgtm!

@edublancas edublancas merged commit f56dfab into ploomber:master Jul 11, 2023
22 checks passed
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.

display improvements to %sqlcmd snippets
3 participants