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

--alias fix #600

Merged
merged 17 commits into from
Jun 14, 2023
Merged

--alias fix #600

merged 17 commits into from
Jun 14, 2023

Conversation

AnirudhVIyer
Copy link

@AnirudhVIyer AnirudhVIyer commented Jun 8, 2023

Describe your changes

Modified the set method in the Connection Class such that: if a connection with a descriptor exists, we check it's alias with the new alias argument.

Issue number

Closes #532

Checklist before requesting a review


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

@AnirudhVIyer AnirudhVIyer marked this pull request as ready for review June 9, 2023 16:06
@AnirudhVIyer
Copy link
Author

@edublancas made the changes and fixed the bug.

However, when i format and lint, it formats and changes the magic-plot.md, magic-sql.md and ggplot-interactive.md.
I made no changes to those files. Tried with a different branch but still got the same result. Has there been any linting changes made on the master branch?

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.

don't worry about the formatting changes (unsure about why it's happening but we can merge it)

just add some unit tests

@edublancas edublancas changed the title alias0fix --alias fix Jun 12, 2023
src/tests/test_connection.py Outdated Show resolved Hide resolved
src/sql/connection.py Show resolved Hide resolved
@anupam-tiwari
Copy link

@AnirudhVIyer I think you removed your code

@AnirudhVIyer
Copy link
Author

@AnirudhVIyer I think you removed your code

Yeah, an integration test is failing, trying to figure that out

@mehtamohit013
Copy link

@AnirudhVIyer
It seems like other PR also have test failing. Make sure you merge with the master and test again

@AnirudhVIyer
Copy link
Author

@AnirudhVIyer It seems like other PR also have test failing. Make sure you merge with the master and test again

@mehtamohit013 I merged with master. The test is still failing, code works on binder though. It was passing all tests until Friday evening.

@edublancas
Copy link

@tonykploomber: can you check the integration tests? looks like they're failing but this PR didn't change them. so perhaps some update in the docker image we're using?

@jinniw43805
Copy link

@edublancas
This is something we found earlier, #614

@edublancas edublancas merged commit 92c40f1 into ploomber:master Jun 14, 2023
@edublancas
Copy link

great work!

@jinniw43805
Copy link

@edublancas
Any idea why tests are skipped when merged to master?

@mehtamohit013
Copy link

mehtamohit013 commented Jun 14, 2023

@edublancas
Any idea why tests are skipped when merged to master?

It may be due to my recently merged PR. In check_doc job, I am taking the git diff of the current branch with the master branch. This works great when running actions for a PR, but when merging, there is no diff between the master latest commit and the current branch i.e. master latest commit. May have to modify the test to run only for PR, and run all the tests when merging

What is your opinion on the above issue? @edublancas @jinniw43805 @tonykploomber

Update: Added a PR. Currently testing

@mehtamohit013 mehtamohit013 mentioned this pull request Jun 14, 2023
4 tasks
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.

confusing behavior when passing --alias
5 participants