ploomber / jupysql

Better SQL in Jupyter. 📊
https://jupysql.ploomber.io
Apache License 2.0
705 stars 75 forks source link

Fix buggy __init__ method #999

Closed rschroll closed 6 months ago

rschroll commented 6 months ago

Describe your changes

It's a silly little thing that I noticed when looking at something else. The facet class in ggplot/facet_wrap.py was missing the self argument in its __init__ method. This wasn't an issue, since this class is never used directly. Only its subclass, facet_wrap is used, which has its own functional __init__ method, which doesn't call the superclass method. Since the method supposed to do anything, I just removed it.

I'm not sure why flake8 or black didn't raise a fuss about this.

Issue number

Closes #X

Checklist before requesting a review


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

rschroll commented 6 months ago

I added a changelog entry in order to let testing continue, but this fix doesn't deserve that! I seem to be unable to add the relevant tag to turn off this requirement.

I've been seeing the failure of test-sql-alchemy-v1 in my own testing. Four tests are failing, all related to persisting with DuckDB. They complain about needing a list of parameters, and they seem to be provided with a (nested) tuple of parameters. This seems completely unrelated to my change. I wonder if some requirement released a new version, and now it's pickier about data types.

The Py3.9 on Windows failure is about some .tcl file not being present. No idea what's up there, but again it seems completely unrelated to these changes.

The integration test seemed to fail at the very end, with errors about what seem like internal tables not existing. Could an over-eager teardown have started before the tests were done?

edublancas commented 6 months ago

I've added the no-changelog label, can you remove the changelog entry? once that's done, we can merge. as you suggested, the test issues are not related to your change

rschroll commented 6 months ago

I force-pushed to the branch to remove the changelog commit. Hope that didn't break anything too badly.

edublancas commented 6 months ago

thanks for your contribution!