Skip to content

eJWST release#2238

Merged
bsipocz merged 5 commits into
astropy:mainfrom
esdc-esac-esa-int:ejwst_release
Dec 6, 2021
Merged

eJWST release#2238
bsipocz merged 5 commits into
astropy:mainfrom
esdc-esac-esa-int:ejwst_release

Conversation

@jespinosaar

Copy link
Copy Markdown
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

codecov Bot commented Dec 3, 2021

Copy link
Copy Markdown

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

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

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.

Comment thread astroquery/esa/jwst/core.py Outdated


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

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread CHANGES.rst Outdated
@jespinosaar

Copy link
Copy Markdown
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

bsipocz commented Dec 6, 2021

Copy link
Copy Markdown
Member

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?

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

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

bsipocz commented Dec 6, 2021

Copy link
Copy Markdown
Member

Thanks @jespinosaar!

@jespinosaar

Copy link
Copy Markdown
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.

2 participants