-
Notifications
You must be signed in to change notification settings - Fork 0
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
Normalize paths in packet metadata. #46
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
- Coverage 99.28% 98.70% -0.58%
==========================================
Files 45 45
Lines 3351 3480 +129
Branches 527 337 -190
==========================================
+ Hits 3327 3435 +108
- Misses 21 39 +18
- Partials 3 6 +3 ☔ View full report in Codecov by Sentry. |
508266a
to
507ca51
Compare
@@ -200,3 +200,30 @@ def openable_temporary_file(*, mode: str = "w+b", dir: Optional[str] = None): | |||
os.unlink(f.name) | |||
except OSError: | |||
pass | |||
|
|||
|
|||
@overload |
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.
are these bits just for mypy?
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.
Yeah, it makes it possible to describe that the output type depends on the input. Might be overkill in this context
join("shared_data", "numbers.txt"): join("data", "numbers.txt"), | ||
join("shared_data", "weights.txt"): join("data", "weights.txt"), | ||
"shared_data/numbers.txt": "data/numbers.txt", | ||
"shared_data/weights.txt": "data/weights.txt", |
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.
is it worth adding something that checks (perhaps on cleanup of a temporary orderly directory) that we never get backslashes anywhere?
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.
How about we do that as part of the JSON schema? We can have a regex on the path-like fields.
Wouldn't be the friendliest of error messages though.
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 idea, because it will get caught in the implementations at least
The packet metadata keeps a record of what files are used and/or produced by the report. When such files are directories, we want to normalize the path separator. This will ensure that a packet produced on Windows can be used on POSIX platforms and vice-versa. Additionally, R (and therefore orderly2) uses forward slashes on all platforms, including Windows. Using forward slashes in outpack-py means packets are compatible and consistent across the two tools. It's not obvious whether the functions that return copied paths to the caller (eg. `orderly.shared_resource`) should return native paths or POSIX paths. For now I've kept these as native paths.
The packet metadata keeps a record of what files are used and/or produced by the report. When such files are directories, we want to normalize the path separator. This will ensure that a packet produced on Windows can be used on POSIX platforms and vice-versa.
Additionally, R (and therefore orderly2) uses forward slashes on all platforms, including Windows. Using forward slashes in outpack-py means packets are compatible and consistent across the two tools.
It's not obvious whether the functions that return copied paths to the caller (eg.
orderly.shared_resource
) should return native paths or POSIX paths. For now I've kept these as native paths.