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
DM-37523: Implement walk() for HttpResourcePath class #41
Conversation
Codecov ReportBase: 84.14% // Head: 84.32% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 84.14% 84.32% +0.18%
==========================================
Files 27 27
Lines 3476 3529 +53
Branches 714 735 +21
==========================================
+ Hits 2925 2976 +51
+ Misses 435 434 -1
- Partials 116 119 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Looks great. Thanks. Minor comments.
url, | ||
), | ||
mem_usage=True, | ||
mem_unit=u.mebibyte, |
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 you worried about memory usage?
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.
The intention is that we collect as much resource consumption information as we can when we are timing the execution. I thought it would not cost too much since time_this
does not collect the memory usage when the logger is not enable for DEBUG
level, which is the normal production configuration.
@@ -27,6 +27,7 @@ | |||
from typing import TYPE_CHECKING, BinaryIO, Iterator, List, Optional, Tuple, Union, cast | |||
|
|||
import requests | |||
from astropy import units as u |
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 requires that we add astropy
to the https
optional dependencies section in pyproject.toml
.
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.
Added "astropy >= 4.0"
, following what is done in https://github.com/lsst/daf_butler/blob/main/pyproject.toml
tests/test_http.py
Outdated
@@ -227,10 +228,26 @@ def test_dav_as_local(self): | |||
self.assertEqual(ResourcePath(local_path).read(), contents) | |||
os.remove(local_path) | |||
|
|||
def test_dav_size(self): | |||
# Size of an inexistent file must raise |
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.
"a nonexistent file" (also, period at end of comment)
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.
Fixed.
Checklist
doc/changes