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.

Sign up for GitHub

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

Jump to bottom

Use display module when generating CTE #683

Merged
merged 8 commits into from
Jul 5, 2023
Merged

Use display module when generating CTE #683

merged 8 commits into from
Jul 5, 2023

Conversation

tl3119
Copy link

@tl3119 tl3119 commented Jun 28, 2023 •
edited by yafimvo
Loading

Describe your changes

Remove the print message.
Use display module to show the message instead when generating CTE

Issue number

Closes #649

Checklist before requesting a review

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

@yafimvo
Copy link

yafimvo commented Jun 28, 2023

@tl3119 Is it ready for review? If so, please make sure it passes the CI.

@tl3119 tl3119 marked this pull request as draft June 28, 2023 14:23
@tl3119 tl3119 marked this pull request as ready for review June 28, 2023 21:20
@tl3119
Copy link
Author

tl3119 commented Jun 28, 2023

@tl3119 Is it ready for review? If so, please make sure it passes the CI.

It is ready for review now.

edublancas
edublancas requested changes Jun 28, 2023
View reviewed changes
src/sql/magic.py Outdated Show resolved Hide resolved
@edublancas edublancas mentioned this pull request Jun 28, 2023
Closed
@tl3119 tl3119 requested a review from edublancas June 28, 2023 23:49
tonykploomber
tonykploomber suggested changes Jun 29, 2023
View reviewed changes
Copy link

@tonykploomber tonykploomber left a comment

Choose a reason for hiding this comment

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

typo

CHANGELOG.md Outdated Show resolved Hide resolved
@tonykploomber
Copy link

@tl3119

  1. Let's add some description into Describe your changes, proper description is required to be reviewed
  2. You can click "Resolve Conversation" if you feel the comment is addressed properly

@tl3119
Copy link
Author

tl3119 commented Jun 29, 2023

@tl3119

  1. Let's add some description into Describe your changes, proper description is required to be reviewed
  2. You can click "Resolve Conversation" if you feel the comment is addressed properly

Sure!

edublancas
edublancas requested changes Jun 30, 2023
View reviewed changes
src/sql/magic.py Outdated Show resolved Hide resolved
@tl3119 tl3119 requested a review from edublancas June 30, 2023 17:40
yafimvo
yafimvo approved these changes Jul 2, 2023
View reviewed changes
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.

@tl3119 Next time please remember to include a link to the documentation preview in the PR description

@tl3119
Copy link
Author

tl3119 commented Jul 3, 2023

lgtm.

@tl3119 Next time please remember to include a link to the documentation preview in the PR description

Sure!

@edublancas edublancas merged commit 60c8a32 into ploomber:master Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yafimvo yafimvo yafimvo approved these changes

@edublancas edublancas edublancas approved these changes

@tonykploomber tonykploomber Awaiting requested review from tonykploomber

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

use display module when generrating CTE
4 participants
@tl3119 @yafimvo @tonykploomber @edublancas