Skip to content

FIX: fixed get_log_level when level = 3#346

Merged
adrinjalali merged 2 commits intoskops-dev:mainfrom
EdAbati:fix-get-log-level
Oct 12, 2023
Merged

FIX: fixed get_log_level when level = 3#346
adrinjalali merged 2 commits intoskops-dev:mainfrom
EdAbati:fix-get-log-level

Conversation

@EdAbati
Copy link
Copy Markdown
Contributor

@EdAbati EdAbati commented Apr 19, 2023 •
edited
Loading

When working on #343, I stumbled upon this tiny bug: if one sets the log level equal to 3, the CLI raises a IndexError exception.

Steps to reproduce:

$ skops convert test.pkl -vvv
Traceback (most recent call last):
 ...
IndexError: list index out of range

I added the fix in this PR and also a small test. :)

BenjaminBossan
BenjaminBossan approved these changes Apr 20, 2023
View reviewed changes
Copy link
Copy Markdown
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.

LGTM, thanks for catching this. Ping @E-Aho

@jot-s-bindra

This comment was marked as off-topic.

Sign in to view
@EdAbati
Copy link
Copy Markdown
Contributor Author

EdAbati commented Oct 12, 2023

HI @jot-s-bindra , I am happy to help!
since it is not related to this PR, could you make an new issue about the problem you are having? We will move the conversation over there :)

@BenjaminBossan
Copy link
Copy Markdown
Collaborator

BenjaminBossan commented Oct 12, 2023

@adrinjalali Feel free to merge, I think this one just slipped under the radar.

@adrinjalali adrinjalali merged commit a6c1ae9 into skops-dev:main Oct 12, 2023
@EdAbati EdAbati deleted the fix-get-log-level branch October 12, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adrinjalali adrinjalali adrinjalali approved these changes

@BenjaminBossan BenjaminBossan BenjaminBossan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@EdAbati @jot-s-bindra @BenjaminBossan @adrinjalali