-
Notifications
You must be signed in to change notification settings - Fork 76
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
Refactor close-matching when reading toml config #976
Refactor close-matching when reading toml config #976
Conversation
@neelasha23 @bryannho please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local testing works fine and code looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, functionality is fine but before we can merge you'll need to add an entry to CHANGELOG.MD
or add the no-changelog
label and then re-run the CI!
Please add a Changelog entry and ensure all the tests are passing @maciejb |
@maciejb is this ready for review? don't worry about the check-for-broken-links test |
The one failing CI test seems to be a prior issue unrelated to my changes, a false-positive on a broken link in |
@bryannho oops, just saw this after I traced back the check-for-broken-links test :) Yes, ready for review, with exception of I don't know whether or not to address @neelasha23's comment here. I do think removing this test is the right to do (not xfail), but whether to replace it with a different test I'm not sure about. |
@maciejb let us know if you still have time to address the review! |
@edublancas , thanks for the ping. Sorry, this turned from a simple "remove extraneous and buggy functionality" into something a little different. Got busy with work and lost track of this. I'll try to throw something together shortly. |
sounds good! |
313bb5f
to
68504bb
Compare
Thanks all, I believe this is now ready to go! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual testing works great, code looks good to me. Thanks for your contribution @maciejb !
thanks for working on this @maciejb! |
Describe your changes
This PR refactors
sql.util.get_user_configs()
to more specifically target only the config section[tool.jupysql.SqlMagic]
.When searching for "jupysql", a hint (instead of an error) will be displayed only if there is exact case-insensitive match (such as "jupySql"). We are making this stricter, because we cannot anticipate the naming of other tools in the general
tool
namespace.A
close_match
is still used for "SqlMagic", so we detect things like "SqlMagics". If found, such things will continue to throw an error with a hint, because we are now in thejupysql
namespace.Because the logic differs slightly at each level, I revised the structure to be a direct linear derival of the key we need, rather than the more generic toml loop search implementation. This enhances readability over the alternative of adding if/else logic within the loop to vary the logic as we go deeper down the tree.
Issue number
Closes #975
Closes #969 -- this is fixed by the
return None, None
at the end. Although not original target of this PR, it wasn't possible to just leave this brokenChecklist before requesting a review
pkgmt format
📚 Documentation preview 📚: https://jupysql--976.org.readthedocs.build/en/976/