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

Improve parsing logic around << #662

Merged
merged 31 commits into from
Jul 13, 2023

Conversation

anupam-tiwari
Copy link

@anupam-tiwari anupam-tiwari commented Jun 24, 2023

Describe your changes

Issue number

Closes #610

Checklist before requesting a review

  • Performed a self-review of my code
  • Formatted my code with pkgmt format
  • Added tests (when necessary).

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

@anupam-tiwari
Copy link
Author

@edublancas can you please check if this error has occurred before!

@edublancas
Copy link

@anupam-tiwari: yeah, this is the tcl problem I mentioned in the morning meeting. I re-ran the CI, let's see if it passes now. next time the CI fails unexpectedly, check the logs. if you find references to tcl, push an empty commit to re-trigger the CI

src/tests/test_parse.py Show resolved Hide resolved
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.

we're no longer using the ready-for-review tag, please request a review using the github panel on the top right (or tag me)

src/tests/test_parse.py Outdated Show resolved Hide resolved
src/sql/parse.py Outdated Show resolved Hide resolved
@anupam-tiwari
Copy link
Author

anupam-tiwari commented Jun 30, 2023

@edublancas can you please review this one!

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. Added minor comments.

src/tests/test_parse.py Show resolved Hide resolved
src/tests/test_parse.py Show resolved Hide resolved
src/sql/parse.py Outdated Show resolved Hide resolved
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 merge conflicts and request a new review when ready

@anupam-tiwari
Copy link
Author

@edublancas does this look good?

src/sql/parse.py Outdated 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.

I think it looks good. Please address @neelasha23 notes and I think we can merge.

@neelasha23
Copy link

Looks good!

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.

@anupam-tiwari make sure tests pass

@edublancas edublancas merged commit 6d60df9 into ploomber:master Jul 13, 2023
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.

improving << parsing logic
4 participants