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

If jupysql config is not present in pyproject.toml, jupysql attemps to read other tools' sections #969

Closed
maciejb opened this issue Dec 22, 2023 · 4 comments · Fixed by #976

Comments

@maciejb
Copy link

maciejb commented Dec 22, 2023

What happens?

When there is no jupysql configuration present, jupysql seems to attempt parsing sections belonging to other tools:

image

Adding a [tool.jupysql.SqlMagic] resolves this

To Reproduce

Create a pyproject.toml containing sections for another tool, but not jupysql. For example:

[tool.isort]
profile = "black"

Load jupysql. Observe a message is emitted such as:

'isort' is an invalid configuration. Please review our configuration guideline.

OS:

macOS

JupySQL Version:

0.10.6

Full Name:

Maciej Bukczynski

Affiliation:

Darkhorse Analytics

@edublancas
Copy link

this is definitely a bug, thanks for reporting! if you have some time, we'd appreciate a PR!

@maciejb
Copy link
Author

maciejb commented Dec 30, 2023

Thanks @edublancas! I'm happy to put something together. As I look through this code, however, I am tempted to make some fairly large changes, so I want to be cautious given this is a project that's new to me... Specifically, the code causing this issue has a fair bit of complexity to accommodate the "did you mean ...?" fuzzy matching. It seems unnecessary when there is one and only one section ([tool.jupysql.SqlMagic]) that is used by jupysql, and other sections in the file are unrelated to jupysql so it has no business judging their validity...

My first suggestion would be to remove the fuzzy matching altogether, completely ignore sections that don't begin with tool.jupysql, and instead of using difflib.get_close_matches(), simply issue a predetermined warning if something starts with tool.jupysql but is not SqlMagics. It seems overkill to use difflib when the only resulting message would ever be, "did you mean SqlMagic?".

Does that sound like a good approach, or is there something I am missing / a desire to retain difflib in config parsing for another reason?

@edublancas
Copy link

@maciejb the idea of fuzzy matching is to validate keys inside tool.jupysql.SqlMagic, so if it's validating anything outside, it's an implementation problem. can you check if that's the case (that we're fuzzy matching stuff outside tool.jupysql.SqlMagic, and if so, please open an issue).

For this PR, i'd suggest sticking with fixing the issue at hand, but if you think that you absolutely need to change the fuzzy matching logic first, feel ree to open a PR to fix that first, and then a second one to fix the issue we're discussing here

@maciejb
Copy link
Author

maciejb commented Jan 3, 2024

Thanks @edublancas , that makes sense. I believe there are two code paths that cause this current issue and one of them would require significant changes to the fuzzy join code, so it would definitely be better to address that first. I'll raise a new issue for that and tackle it first, then come back to this one.

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 a pull request may close this issue.

2 participants