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

Fix buggy __init__ method #999

Merged
merged 1 commit into from
Mar 26, 2024
Merged

Conversation

rschroll
Copy link

@rschroll rschroll commented Mar 23, 2024

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/

It was missing the self argument.  But it also isn't needed at all.
@rschroll
Copy link
Author

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 edublancas added the no-changelog This PR doesn't require a changelog entry label Mar 25, 2024
@edublancas
Copy link

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 rschroll marked this pull request as ready for review March 25, 2024 19:50
@rschroll
Copy link
Author

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

@edublancas edublancas merged commit 4f3c204 into ploomber:master Mar 26, 2024
26 of 31 checks passed
@edublancas
Copy link

thanks for your contribution!

@xofbd xofbd deleted the dev/bad-init branch May 22, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog This PR doesn't require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants