-
Notifications
You must be signed in to change notification settings - Fork 6
Allow customizing env info #214
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
==========================================
+ Coverage 97.29% 97.75% +0.45%
==========================================
Files 3 3
Lines 518 579 +61
Branches 74 77 +3
==========================================
+ Hits 504 566 +62
Misses 5 5
+ Partials 9 8 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
akaszynski
left a comment
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.
This is a great way to add metadata to our images just through the file name. We might also consider adding metadata to the jpeg itself, perhaps containing much of the pyvista.Report(), but having it there in the file name might be enough.
Didn't know that was possible. Will try adding that. I think this will cause GPUInfo to be called twice though, which is not ideal. Here's some sample image names generated from pyvista/pyvista#7914. There's a small issue with "-CI" including a dash when it shouldn't. VTK Dev: macOS Windows |
Will leave this for a separate PR, and after #213 is implemented, because the unit tests save PNGs, but the doc tests save JPGs, so storing the metadata will need to consider both formats. |
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 the responsive implementation
I added a few suggestions, would be happy to contribute if not being clear.
Co-authored-by: beroda <[email protected]>
|
@beroda I believe all comments should now be resolved. |
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.
The env_info name could be improved #214 (comment) but overall LGTM.
Allow customizing info saved in image filenames when using
--generate_subdirs. See #211 (review) for details.Adds a new dataclass with the option to customize it using
verify_image_cache.env_info.