Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Handle optional parameters without implicit bool cast #3502

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

nk-fouque
Copy link
Contributor

@nk-fouque nk-fouque commented Oct 31, 2023

This PR updates the code in tfidfmodel.py that used "if [param]" to check if param had been defined. This method used an implicit casting to boolean to treat None as False, however that posed a few issues.

Mainly when using Pandas Series, which have a particular way of handling boolean casting (raises a warning prompting to use a defined method), despite the fact that they are proper Iterable Objects and should be allowed as parameters.

Other parts of the library already use "if [param] is not None", I just aligned the code to those parts.

Proper unit testing has been run to ensure this is not a breaking change.

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 8, 2024

@piskvorky What are your thoughts on this?

Personally, I'm OK with the code base as-is (explicit is better than implicit, etc.), unless there is a demonstrable use case (e.g. a test case) that shows that it's broken.

@mpenkov mpenkov added this to the Next release milestone Apr 8, 2024
@piskvorky
Copy link
Owner

Sure, makes sense to me. I don't know if this breaks anything deeper down – but hopefully our tests would catch that.

@mpenkov mpenkov modified the milestone: Summer 2024 release Jun 11, 2024
@piskvorky
Copy link
Owner

@mpenkov could you run the test suite? And if all passes, let's merge & release as part of 4.3.3.

@mpenkov
Copy link
Collaborator

mpenkov commented Jun 11, 2024

LGTM. Thank you @nk-fouque !

@mpenkov mpenkov merged commit c95bdb4 into piskvorky:develop Jun 11, 2024
32 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants