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

Adds support to filepath_from_url to support non-encoded URLs #1664

Conversation

douglascomet
Copy link
Contributor

@douglascomet douglascomet commented Sep 24, 2023

filepath_from_url works for encoded URLs but not for URLs that are not encoded. I encountered this when working with URLs from both .xml and .aaf files.

The .xml file came from Premiere and encoded the URL while the .aaf came from Avid and the URL was not encoded.

Examples:

  • "file://localhost/S%3a/path/file.ext" -> "S:/path/file.ext"
  • "file://S:/path/file.ext" -> "/path/file.ext"

My PR updates the code to return the same result with both of the example URLs:

  • "file://localhost/S%3a/path/file.ext" -> "S:/path/file.ext"
  • "file://S:/path/file.ext" -> "S:/path/file.ext"

I tested and dev-ed this on Windows with OTIO 0.15.0 and 0.16.0.dev1.

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2023

Codecov Report

Merging #1664 (e594713) into main (f6a3a9b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1664      +/-   ##
==========================================
+ Coverage   79.84%   79.85%   +0.01%     
==========================================
  Files         197      197              
  Lines       21751    21763      +12     
  Branches     4350     4353       +3     
==========================================
+ Hits        17366    17378      +12     
  Misses       2232     2232              
  Partials     2153     2153              
Flag Coverage Δ
py-unittests 79.85% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/py-opentimelineio/opentimelineio/url_utils.py 100.00% <100.00%> (ø)
tests/test_url_conversions.py 92.85% <100.00%> (+2.38%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6a3a9b...e594713. Read the comment docs.

@douglascomet douglascomet force-pushed the add_support_to_filepath-from-url_for_non_encoded_urls branch from 9617fb0 to 14bad85 Compare September 24, 2023 01:19
@douglascomet douglascomet force-pushed the add_support_to_filepath-from-url_for_non_encoded_urls branch from 14bad85 to fc0900f Compare September 24, 2023 01:24
@meshula
Copy link
Collaborator

meshula commented Sep 24, 2023

Looking at the documentation (https://docs.python.org/3/library/urllib.request.html) , and searching error reports, it seems pretty clear that the urllib has not had a lot of attention around aspects such as drive letters and windows specific encodings. It's disappointing that we need to do this, but I think it's necessary.

Have you filed any issues with urllib around this, as the problem does seem to lie at their end. We should not gate merging this PR on such resolution, but it seems like something needs to happen upstream for the benefit of all.

@apetrynet
Copy link
Contributor

Now that we've left python 2 behind I think we could start using pathlib for some of this as well.

@douglascomet
Copy link
Contributor Author

douglascomet commented Sep 25, 2023

Have you filed any issues with urllib around this, as the problem does seem to lie at their end. We should not gate merging this PR on such resolution, but it seems like something needs to happen upstream for the benefit of all.

@meshula, now that you mention it you're right this is a urllib issue and not OTIO's`. The motivation for this PR was to offload the responsibility of maintaining this code in a custom hook I made and help future developers looking to use OTIO on WIndows. Incorporating this into OTIO seems like the quicker route for now.

I looked through the cpython repo and found these open tickets that seem to relate to this very issue:

Since this issue with cypthon and is still being debated I'm not sure if there is any benefit opening a new issue that is similar to existing ones.

@douglascomet
Copy link
Contributor Author

Now that we've left python 2 behind I think we could start using pathlib for some of this as well.

@apetrynet, what were you thinking about using pathlib here? I also found this closed PR about generating Path from a URL, python/cpython#91504.

@apetrynet
Copy link
Contributor

Now that we've left python 2 behind I think we could start using pathlib for some of this as well.

@apetrynet, what were you thinking about using pathlib here? I also found this closed PR about generating Path from a URL, python/cpython#91504.

I was thinking about some windows path handling, but looking at it I'm not sure it'll help in this case.

Copy link
Contributor

@apetrynet apetrynet left a comment

Choose a reason for hiding this comment

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

I've provided a suggestion for an alternative way of dealing with this. I'm not sure if it's any cleaner, but I try to rely on the builtin modules to resolve this correctly. Should hopefully adapt to the underlying OS

src/py-opentimelineio/opentimelineio/url_utils.py Outdated Show resolved Hide resolved
tests/test_url_conversions.py Outdated Show resolved Hide resolved
Copy link
Contributor

@apetrynet apetrynet left a comment

Choose a reason for hiding this comment

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

Good call on the comments!
I have a couple of minor requests.

src/py-opentimelineio/opentimelineio/url_utils.py Outdated Show resolved Hide resolved
src/py-opentimelineio/opentimelineio/url_utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@apetrynet apetrynet left a comment

Choose a reason for hiding this comment

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

Thanks @douglascomet !
LGTM

@apetrynet apetrynet merged commit 745e614 into AcademySoftwareFoundation:main Oct 13, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants