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

[python-package] migrate test_plotting.py to pytest #3811

Merged
merged 2 commits into from
Jan 22, 2021

Conversation

thomasjpfan
Copy link
Contributor

Towards #3732

@jameslamb jameslamb changed the title TST Migrates test_plotting.py to pytest [python-package] migrate test_plotting.py to pytest Jan 22, 2021
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks so much for all your help! I just left one small suggestion about linting.

I reviewed with https://www.textcompare.org/index.html?id=600a657520516c001761fc4c.

I like the use of module-scoped features to replicated what we were doing in setUp(), thank you.

Comment on lines 91 to 92
assert (ax1.get_title() ==
'Histogram for feature name {}'.format(gbm1.booster_.feature_name()[27]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert (ax1.get_title() ==
'Histogram for feature name {}'.format(gbm1.booster_.feature_name()[27]))
title = 'Histogram for feature name {}'.format(gbm1.booster_.feature_name()[27])
assert ax1.get_title() == title

I think this change would address the issue from linting ():

Linting Python code
./tests/python_package_test/test_plotting.py:91:29: W504 line break after binary operator

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you!

@jameslamb jameslamb merged commit b638684 into microsoft:master Jan 22, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants