-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[ci] prevent Python tests from leaving behind files #6626
[ci] prevent Python tests from leaving behind files #6626
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
I haven't tested yet if you've caught all the cases where this happens, but these fixes look like what I'd expect. Please see the suggestions I left, and the changes I made to the title and description.
PR titles become commit messages and therefore changelog entries in this repo, so they should describe the changes in a way that would be clear to someone reading the release notes. To see the conventions we follow here, compare these:
- commit log on
master
: https://github.com/microsoft/LightGBM/commits/master/ - most recent release notes: https://github.com/microsoft/LightGBM/releases/tag/v4.5.0
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Co-authored-by: James Lamb <[email protected]>
Noted and adressed your comments @jameslamb thanks for the prompt review |
Please update this branch to the latest You can fix those by running the following from the root of the repo: pip install pre-commit
pre-commit run --all-files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other such occurrences as in #6627 and here? Maybe we can merge all of these changes into a single PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Have now merged #6627 as the total code diff is small and they share the same purpose. I let the moderators close the other PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks very much! I've merged in latest master
to get the changes from #6636, will merge this if CI passes.
We really appreciate the help!
Thanks for the approvals. I am very pleased with this first minor contribution to a package I have been using with enjoyment and satisfaction! Let's see if I can add more to it in the future. |
Contributes to #6361
Prevents the following files from being left behind when the Python tests are run:
data_dask.csv
Tree4.gv
Tree4.gv.pdf
The graphivz artefacts from
tests/python_package_test/test_plotting.py
now go to the pytesttmp_path
and are deleted after test run.