-
Notifications
You must be signed in to change notification settings - Fork 1
Adding tests to verify cloud patterns #107
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #107 +/- ##
=======================================
Coverage 96.72% 96.72%
=======================================
Files 8 8
Lines 214 214
=======================================
Hits 207 207
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
LGTM! thank you, Shubh!
% This list contains path pattern currently supported by Zarr | ||
% in MATLAB. Any invalid path not matching any of these | ||
% patterns will result in an error. | ||
inpPath = {'https://mybucket.s3.us-west-2.amazonaws.com/path/to/myZarrFile', ... |
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.
curious - why cell array of char vectors instead of a more modern array of strings?
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.
Nothing specific.
'https://s3.eu-central-1.example.edu/mybucket/path/to/myZarrFile', ... | ||
's3://mybucket/path/to/myZarrFile'}; | ||
|
||
for i = 1:length(inpPath) |
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.
Very minor and somewhat subjective, but you can use a for-each loop if you don't need the index i for anything else other than getting the current path
paths = ["path1", "path2", "path3"]; % has to be a row vector
for p = paths % iterate over the array of strings directly instead of using index
disp(p)
end
That way you don't have to keep doing {i}
to access the current path. Alternatively, can save the current path to a separate variable first currPath = inpPath{i}
.
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.
Thanks, but keeping it as is for now, for better readability.
Added positive testpoint as discussed offline. The negative test was already as part of a separate testpoint, so I am not adding anything new to verify the error.