astroquery icon indicating copy to clipboard operation
astroquery copied to clipboard

Fix changing `simbad` config items at runtime

Open eerovaher opened this issue 3 years ago • 8 comments

Previously changing simbad config items at runtime had no effect. Now the config items are read when they are needed, unless the user has specified the corresponding class or instance variable, in which case the latter takes precedence.

Relevant for #2291

eerovaher avatar Feb 10 '22 16:02 eerovaher

Codecov Report

Merging #2292 (0f6a310) into main (0883e92) will increase coverage by 0.01%. The diff coverage is 95.23%.

@@            Coverage Diff             @@
##             main    #2292      +/-   ##
==========================================
+ Coverage   62.99%   63.01%   +0.01%     
==========================================
  Files         133      133              
  Lines       17291    17300       +9     
==========================================
+ Hits        10892    10901       +9     
  Misses       6399     6399              
Impacted Files Coverage Δ
astroquery/simbad/core.py 90.84% <95.23%> (+0.19%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Feb 10 '22 17:02 codecov[bot]

I had to rebase to solve a conflict in the change log.

eerovaher avatar Feb 14 '22 14:02 eerovaher

I had to rebase again to resolve another merge conflict in the change log.

I also decided to add a _row_limit property that works analogously to the other two I had added earlier. The main reason I added the properties was to avoid code duplication and adding a property for row limit was not needed for that. But for consistency and simplicity it is better if there are properties for all the config items.

eerovaher avatar Feb 22 '22 17:02 eerovaher

I would like to get this change done in the same release for a bunch of the modules (at least the ones that are most often used), and maybe also add the warning #638 suggest (there is one already in mast.missions), so would push this to the next release. The plan is to get that out reasonably quickly, and the plan also includes quite a few backend changes, e.g. caching, cloud access, etc, so this would perfectly fit in there.

bsipocz avatar Mar 18 '22 01:03 bsipocz

I have rebased the commits to update the change log entry.

eerovaher avatar Mar 24 '22 17:03 eerovaher

For some reason none of the tests ran at all. Should I rebase again to start the tests?

eerovaher avatar Mar 29 '22 19:03 eerovaher

I've updated the pull request so that the documentation about the Simbad subpackage is kept up to date regarding the configuration, and I had to rebase to resolve a merge conflict in the change log.

eerovaher avatar Jul 27 '22 20:07 eerovaher

I had to rebase to fix a merge conflict in the change log, again.

eerovaher avatar Aug 26 '22 19:08 eerovaher