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

eJWST release #2238

Merged
merged 5 commits into from
Dec 6, 2021
Merged

eJWST release #2238

merged 5 commits into from
Dec 6, 2021

Conversation

jespinosaar
Copy link
Contributor

Dear @bsipocz , @keflavich:

Now we are ready for the operational release of eJWST astroquery module, so, as we agreed, I have uploaded the operational URLs. Thanks for your support!

@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #2238 (9667831) into main (11c2ddf) will increase coverage by 0.17%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2238      +/-   ##
==========================================
+ Coverage   62.20%   62.38%   +0.17%     
==========================================
  Files         131      131              
  Lines       16772    16777       +5     
==========================================
+ Hits        10433    10466      +33     
+ Misses       6339     6311      -28     
Impacted Files Coverage Δ
astroquery/esa/jwst/core.py 75.65% <73.33%> (+5.98%) ⬆️
astroquery/utils/tap/core.py 41.42% <0.00%> (-0.20%) ⬇️
astroquery/utils/tap/conn/tapconn.py 47.60% <0.00%> (+1.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11c2ddf...9667831. Read the comment docs.

@bsipocz bsipocz added this to the v0.4.5 milestone Dec 3, 2021
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Adding some non mocked remote tests would be great, otherwise I have one comment to address and this is good to go.

@@ -1239,4 +1241,7 @@ def get_decoded_string(str):
return str


Jwst = JwstClass()
if "pytest" in sys.modules:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's right to have this here in the main code, all testing stuff should be handled in the test files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, I was thinking on how to avoid connection errors on this, now I have found a workaround using try/except in get_status_messages method

CHANGES.rst Outdated Show resolved Hide resolved
@jespinosaar
Copy link
Contributor Author

Hi @bsipocz, I have solved your comment about pytest in the main code, and added some additional tests. Regarding remote-data tests, as there will be data after launch in the archive, but not right now, we will include them when we have some static datasets to be used in the tests. Please do not hesitate to contact me in case you have further comments.

On the other hand, I would like to ask you about the release of v0.4.5, after this code has been merged. When do you think we could have this version available?

@bsipocz
Copy link
Member

bsipocz commented Dec 6, 2021

When do you think we could have this version available?

We said it should be out by the launch. Is that still a good timeline?

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

OK, let's get this in now. if needed, incremental smaller PRs are more than welcome (one is certainly needed to remove the disclaimer from the docs once the archive is fully operational).

@bsipocz bsipocz merged commit 54ca5fe into astropy:main Dec 6, 2021
@bsipocz
Copy link
Member

bsipocz commented Dec 6, 2021

Thanks @jespinosaar!

@jespinosaar
Copy link
Contributor Author

Thanks a lot @bsipocz ! I am opening a new PR for the disclaimer: #2243, there I have answered your question about the timeline and the disclaimer.

@jespinosaar jespinosaar deleted the ejwst_release branch December 16, 2021 12:18
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

2 participants