Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Consolidate filename normalization for exports #2782

Merged
merged 6 commits into from
Apr 3, 2024

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Apr 2, 2024

This consolidates the filename logic for exporting to fix the standalone case for all export types, not just movies. I wasn't sure whether to put the changelog in with the other Export stuff or in Bug Fixes, since I think the standalone issue was present in 3.8.

@rosteen rosteen added bug Something isn't working plugin Label for plugins common to multiple configurations labels Apr 2, 2024
@rosteen rosteen added this to the 3.9 milestone Apr 2, 2024
@pllim
Copy link
Contributor

pllim commented Apr 2, 2024

Looks like one test case broke:

cubeviz/plugins/tests/test_export_plots.py::test_export_movie_cubeviz_exceptions - AttributeError: 'str' object has no attribute 'parent'

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.82%. Comparing base (f1ba4a6) to head (09f4b85).
Report is 16 commits behind head on main.

❗ Current head 09f4b85 differs from pull request most recent head 5f5d88a. Consider uploading reports for the commit 5f5d88a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2782      +/-   ##
==========================================
+ Coverage   88.72%   88.82%   +0.09%     
==========================================
  Files         110      110              
  Lines       16353    16425      +72     
==========================================
+ Hits        14509    14589      +80     
+ Misses       1844     1836       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rosteen rosteen mentioned this pull request Apr 2, 2024
8 tasks
@rosteen
Copy link
Collaborator Author

rosteen commented Apr 3, 2024

@pllim I fixed that test, the remaining failure is a failed remote data download, not anything to do with this PR.

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

This makes sense to me and also am glad to see the filetype handling reusable rather than copy-pasted everywhere. Thanks!

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Nice clean up. Thanks!

jdaviz/configs/default/plugins/export/export.py Outdated Show resolved Hide resolved
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@rosteen rosteen enabled auto-merge (squash) April 3, 2024 14:13
@rosteen rosteen merged commit dd567d4 into spacetelescope:main Apr 3, 2024
14 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants