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

Fix Cubeviz movie export #2942

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Jul 8, 2024

There was a line pertaining to Path filenames, but _normalize_filename now turns everything into a string. Exporting movies works for me now.

Not sure what we're milestoning things at at this point...

@rosteen rosteen added bug Something isn't working cubeviz labels Jul 8, 2024
@github-actions github-actions bot added the plugin Label for plugins common to multiple configurations label Jul 8, 2024
@rosteen rosteen added this to the 3.10.3 milestone Jul 8, 2024
@pllim
Copy link
Contributor

pllim commented Jul 8, 2024

I don't see any resolve() calls in _normalize_filename and I forgot why we need it. Are you sure we don't anymore?

@pllim pllim added the 💤backport-v3.10.x on-merge: backport to v3.10.x label Jul 8, 2024
@rosteen
Copy link
Collaborator Author

rosteen commented Jul 8, 2024

I don't see any resolve() calls in _normalize_filename and I forgot why we need it. Are you sure we don't anymore?

I added it back in, it probably isn't needed for any of our cases but it's good to have it in to handle some weirder edge cases of what people might input in the field, I think.

Copy link
Contributor

@gibsongreen gibsongreen left a comment

Choose a reason for hiding this comment

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

Videos are now downloading with content!

@pllim
Copy link
Contributor

pllim commented Jul 8, 2024

@gibsongreen , is the recording "cutting off" still an issue at all?

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.

LGTM. Thanks!

@pllim pllim enabled auto-merge (squash) July 8, 2024 19:40
@pllim
Copy link
Contributor

pllim commented Jul 8, 2024

@rosteen , if auto merge fails, take it off auto merge and use your admin power.

@pllim pllim merged commit 0d7684a into spacetelescope:main Jul 8, 2024
21 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/jdaviz that referenced this pull request Jul 8, 2024
rosteen added a commit that referenced this pull request Jul 8, 2024
Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>
@gibsongreen
Copy link
Contributor

@gibsongreen , is the recording "cutting off" still an issue at all?

@pllim The two issues I had seen was 1, on main that videos were not exporting at all, and 2, after #2896 only blank videos. This was for my local system so maybe cutting off was happening for someone else but this PR enabled the slice range I selected to download as I would expect

javerbukh pushed a commit to javerbukh/jdaviz that referenced this pull request Jul 16, 2024
* Remove line relating to Path, filename now normalized to str

* Add resolve() call to filename normalization, changelog
Copy link

codecov bot commented Oct 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.79%. Comparing base (0ffae63) to head (4a207be).
Report is 153 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2942   +/-   ##
=======================================
  Coverage   88.79%   88.79%           
=======================================
  Files         111      111           
  Lines       17223    17234   +11     
=======================================
+ Hits        15293    15303   +10     
- Misses       1930     1931    +1     

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working cubeviz plugin Label for plugins common to multiple configurations Ready for final review 💤backport-v3.10.x on-merge: backport to v3.10.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants