Fix atomic caching infinite loop #2339
Conversation
…ts to avoid the infinite loop in astropy#2338
astroquery/atomic/core.py
Outdated
| self.__default_form_values = self._get_default_form_values(form) | ||
|
|
||
| return self.__default_form_values | ||
| response = self._request("GET", url=self.FORM_URL, params={}, |
There was a problem hiding this comment.
you changed data={} to params={} here... I think either way, we could just exclude this? It's a very minor thing but I'm not sure what purpose it serves.
There was a problem hiding this comment.
This was indeed a red herring for the problem but I kept it as it's still a fix even as it's an empty dict so it shouldn't matter in practice. (afaik data doesn't have any effect for a GET query, it takes a params dict (while POST can have either or both).)
…on remote-data accessing testing
|
So, with the latest commit I needed to bring back not setting the default values at initialization to make sure we're not reaching the internet at init time, to allow non-remote tests to pass. Not ideal, but it works, and honestly I don't expect this module to be any big data bottleneck, so probably not worth spending much more time on finding a more optimal solution. |
Codecov Report
@@ Coverage Diff @@
## main #2339 +/- ##
==========================================
- Coverage 63.41% 63.40% -0.02%
==========================================
Files 131 131
Lines 17043 17045 +2
==========================================
- Hits 10808 10807 -1
- Misses 6235 6238 +3
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
Given the thumbs up and approval above, I go ahead and merge this, to get one more out of the way... |
|
🤦♀️ - I didn't rerun the remote tests :( |
|
The fix that removes the extraneous lines used for debugging is in #1937. With that fix all atomic tests pass on that PR, and thus it's good to go. |
This is to fix #2338. I made atomic commits to make the motivation clear for each individual fix, as this PR fixes a few separate issues.
#1937 can be merged after this one, it passed locally on top of this branch.
I'll add a changelog once we're happy with the scope of this.