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

MNT add helper functions to convert types to strings in .audit #215

Conversation

E-Aho
Copy link
Collaborator

@E-Aho E-Aho commented Nov 27, 2022

A few minor changes to the new audit functionality that allows us to pass in types as well as strings as defaults.

This PR adds in a few helper functions to convert from types to their namespace strings, and a helper function that can convert any non-string defaults in a passed in list to their namespace.

Also includes new tests that cover this added functionality

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty good overall, thanks. I only have minor comments, please take a look.

PS: I didn't see Adrin's review when I wrote this one.

skops/io/_dispatch.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/tests/test_utils.py Show resolved Hide resolved
skops/io/tests/test_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

skops/io/_dispatch.py Outdated Show resolved Hide resolved
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have a separate PR to work on the public API allowing users to pass types.

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks, great, this PR can be merged, there are just two minor improvements for docstrings that I would suggest.

skops/io/_utils.py Outdated Show resolved Hide resolved
skops/io/_utils.py Outdated Show resolved Hide resolved
E-Aho and others added 5 commits December 1, 2022 10:18
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
@adrinjalali
Copy link
Member

you might want to enable pre-commit hooks, which include isort ;)

@E-Aho
Copy link
Collaborator Author

E-Aho commented Dec 1, 2022

I've got that all set up on my personal computer, trying to wrestle through it on my work laptop atm to get this merged lol

@E-Aho
Copy link
Collaborator Author

E-Aho commented Dec 1, 2022

This is a good reminder to me how nice it is to have pre-commit hooks 🙃 Should be good to go now

@adrinjalali adrinjalali changed the title ENH: add helper functions to convert types to strings in .audit MNT add helper functions to convert types to strings in .audit Dec 1, 2022
@adrinjalali adrinjalali merged commit 7242265 into skops-dev:main Dec 1, 2022
@adrinjalali
Copy link
Member

Note: I change the commit title to MNT since this doesn't expose any changes to the user. I feel like ENH has user-facing implications.

@E-Aho E-Aho deleted the ENH-Add-helper-functions-to-convert-types-to-strings branch December 1, 2022 11:13
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.

3 participants