Skip to content
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

Fix changing simbad config items at runtime #2292

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eerovaher
Copy link
Member

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

@codecov
Copy link

codecov bot commented Feb 10, 2022

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%) ⬆️

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

@ceb8 ceb8 added this to the v0.4.6 milestone Feb 13, 2022
@ceb8 ceb8 added gaia simbad and removed gaia labels Feb 13, 2022
Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this solution. @bsipocz I'm holding off on pressing merge so you can have a look, but I think this could serve as the template for other modules moving forward.

I would also like to see these parameters also be specifiable at instance creation time, but that can be a conversation on a different thread.

eerovaher added a commit to eerovaher/astroquery that referenced this pull request Feb 14, 2022
@eerovaher
Copy link
Member Author

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

eerovaher added a commit to eerovaher/astroquery that referenced this pull request Feb 22, 2022
@eerovaher
Copy link
Member Author

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.

@bsipocz
Copy link
Member

bsipocz commented Mar 18, 2022

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 bsipocz modified the milestones: v0.4.6, v0.4.7 Mar 18, 2022
eerovaher added a commit to eerovaher/astroquery that referenced this pull request Mar 24, 2022
@eerovaher
Copy link
Member Author

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

@eerovaher
Copy link
Member Author

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

eerovaher added a commit to eerovaher/astroquery that referenced this pull request Jul 26, 2022
eerovaher added a commit to eerovaher/astroquery that referenced this pull request Jul 27, 2022
eerovaher added a commit to eerovaher/astroquery that referenced this pull request Jul 27, 2022
@eerovaher
Copy link
Member Author

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.

Currently changing Simbad config values at runtime has no effect. The
added test reveals the problem.
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.
@eerovaher
Copy link
Member Author

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

@bsipocz bsipocz removed this from the v0.4.7 milestone Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants