Skip to content

Conversation

@luyaor
Copy link
Contributor

@luyaor luyaor commented Jan 11, 2021

See #7241

@luyaor
Copy link
Contributor Author

luyaor commented Jan 11, 2021

@jwfromm

@jwfromm
Copy link
Contributor

jwfromm commented Jan 12, 2021

Can you add a test that would trigger the error? Once that's included in this PR it should be good to go. Thanks for identifying and fixing this.

@tqchen
Copy link
Member

tqchen commented Feb 16, 2021

ping @jwfromm @luyaor , it would be great to follow up on this

@tqchen tqchen added the status: need test case need test cases to cover the change label Feb 16, 2021
@luyaor
Copy link
Contributor Author

luyaor commented Mar 2, 2021

Hi @jwfromm , I added a testcase.

Copy link
Contributor

@jwfromm jwfromm 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 this fix!

@jwfromm
Copy link
Contributor

jwfromm commented Mar 2, 2021

Looks like you just need to format the test file and we can merge.

@luyaor
Copy link
Contributor Author

luyaor commented Mar 8, 2021

Hi @jwfromm , what do you mean that format the test file? I am a little bit confused.

@tqchen
Copy link
Member

tqchen commented Mar 8, 2021

run tests/lint/git-black.sh -i upstream/main to format the python code so we can pass the CI lint error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: need test case need test cases to cover the change status: review in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants