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

Small refactoring changes to USBBinds.py #226

Merged
merged 6 commits into from
May 28, 2024
Merged

Conversation

mjmucha
Copy link
Member

@mjmucha mjmucha commented May 27, 2024

This PR includes mostly small refactoring changes. It also includes a fix in line 109 (query_identification() does not have the argument verbose anymore).

@cbespin
Copy link
Contributor

cbespin commented May 27, 2024

Do you need any verbose arguments if you use 90% of them with log.info anyway? I would claim this is the preferred way, because if users want verbose info, they should enable logging on debug level. But I am open to discussions.

@mjmucha
Copy link
Member Author

mjmucha commented May 27, 2024

I'm kinda used to implement some functions with verbose arguments, due to how e.g. scikit-learn and other machine learning libraries are often structured. However, I really have no preference here and am open to changing the code.

@cbespin
Copy link
Contributor

cbespin commented May 27, 2024

Sounds like we need a third opinion @YannickDieter @leloup314 :D

basil/utils/USBBinds.py Outdated Show resolved Hide resolved
@leloup314
Copy link
Member

I think one should stick to logging for this use case. There is a reason scikit-learn use a dedicated keyword argument for this but its overkill here.

@mjmucha
Copy link
Member Author

mjmucha commented May 28, 2024

The PR now uses purely logging and should be ready for merging.

@mjmucha mjmucha changed the title Small refactoting changes to USBBinds.py Small refactoring changes to USBBinds.py May 28, 2024
Copy link
Contributor

@cbespin cbespin left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me now

@mjmucha mjmucha merged commit cd3497c into master May 28, 2024
5 checks passed
@mjmucha mjmucha deleted the usbbinds_enhacements branch May 28, 2024 11:10
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