Use url_utils.url_from_filepath() for file:// URLs in docs/examples#2033
Open
anAirdrop wants to merge 2 commits into
Open
Use url_utils.url_from_filepath() for file:// URLs in docs/examples#2033anAirdrop wants to merge 2 commits into
anAirdrop wants to merge 2 commits into
Conversation
The adapter-writing tutorial and two example scripts (conform.py, shot_detect.py) built target_url values by concatenating "file://" directly onto a filesystem path. Per RFC 3986, this only produces a correct file URI when the path is POSIX-absolute; for a relative path or a Windows drive-letter path (e.g. "C:/show/movie.mov"), the first path segment is parsed as the URL's host/netloc, silently corrupting the URL (e.g. "file://C:/show/movie.mov" instead of the correct "file:///C:/show/movie.mov"). opentimelineio.url_utils.url_from_filepath() already handles this correctly (absolute vs. relative paths, Windows drive letters, UNC paths) and is covered by tests/test_url_conversions.py. This change points the docs and examples at that existing utility instead of hand-rolling the URL.
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2033 +/- ##
=======================================
Coverage 83.21% 83.21%
=======================================
Files 180 180
Lines 13465 13465
Branches 1253 1253
=======================================
Hits 11205 11205
Misses 2088 2088
Partials 172 172
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Collaborator
|
@anAirdrop Thanks for the fixes, these look great! Would you mind just addressing the DCO signoff issue and then I think we should be able to merge! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the
target_urlexamples indocs/tutorials/write-an-adapter.mdand two example scripts (examples/conform.py,examples/shot_detect.py) that builtfile://URLs by string-concatenating"file://" + path. This only produces a valid URI for POSIX-absolute paths; for relative or Windows drive-letter paths, the first path segment is silently parsed as the URL's host (RFC 3986), producing a corrupted URL.opentimelineio.url_utils.url_from_filepath()already handles this correctly (seetests/test_url_conversions.py), this PR just points the docs and examples at it instead of hand-rolling the URL.Related to #1985. This is the narrow docs/examples fix mentioned in that thread, scoped separately from the larger Application Integrator's Guide rewrite being discussed there.
Test plan
url_utils.pyitself is unchanged and already covered bytests/test_url_conversions.py.examples/conform.pyandexamples/shot_detect.pyhave no existing dedicated test coverage (they are standalone CLI examples, not part of the installed package), did not want to expand the scope of this PR to add new test infrastructure for the examples directory.python -m py_compileon both edited example scripts.