BUG: fix for alma timeout #2535
Conversation
|
This PR also includes fixes for something unrelated, but was necessary to pass all the tests. Can someone advise on how to proceed with that? Are the fixes better off in a separate Issue? |
Codecov Report
@@ Coverage Diff @@
## main #2535 +/- ##
==========================================
+ Coverage 63.18% 63.21% +0.02%
==========================================
Files 133 133
Lines 17236 17250 +14
==========================================
+ Hits 10891 10905 +14
Misses 6345 6345
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
astroquery/alma/core.py
Outdated
| if p.ID == parameter_id: | ||
| return p.value | ||
|
|
||
| return None |
There was a problem hiding this comment.
A trailing return or return None is redundant
EDIT: removed suggestion
There was a problem hiding this comment.
PEP 8 says:
... If any return statement returns an expression, any return statements where no value is returned should explicitly state this as
return None, and an explicit return statement should be present at the end of the function (if reachable)...
There was a problem hiding this comment.
Huh. Thanks. This is the first time I think I really disagree with pep8, but ok.
There was a problem hiding this comment.
Reading a bit up on it, yes, maybe we need to add these back for all over the library, but atm pycodestyle/flake8 is not complaining, and if we run and check something more strict like pylint, we'll suffocating under linter errors.
So I removed the line (after all the only aim for this particular PEP8 rule is to be consistency, and removing this line is in fact more consistent with the rest of the library), and will merge once CI pass.
| @@ -518,8 +522,8 @@ def run_sync(self, uid): | |||
| alma._datalink = MockDataLinkService() | |||
| result = alma.get_data_info(uids='uid://A001/X12a3/Xe9', expand_tarfiles=True) | |||
|
|
|||
| # Entire expanded structure is 61 links long. | |||
| assert len(result) == 61 | |||
| # Entire expanded structure is 19 links long. | |||
There was a problem hiding this comment.
Is this something that will change? If so, add the date in a comment. If not, it's fine to leave this as-is - but it would be good to leave some note about what to do if the number changes.
There was a problem hiding this comment.
the date is in the commit, so I left this as is for now. If it starts failing we can think about modifications.
bsipocz
force-pushed
the
alma-timeouts
branch
from
d7922c0
to
170d23c
Compare
|
Thank you @at88mph! |
Add timeouts and test fixes.
DataLink tests were failing to traverse the ad-hoc DataLink services. I made some test and code fixes in order to push through the timeout changes.