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

A number of CADC authentication and doc fixes #2374

Merged
merged 9 commits into from
Apr 27, 2022

Conversation

andamian
Copy link

@andamian andamian commented Apr 25, 2022

@bsipocz bsipocz added the cadc label Apr 25, 2022
@bsipocz bsipocz added this to the v0.4.7 milestone Apr 25, 2022
@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #2374 (65627ee) into main (1f4e665) will increase coverage by 0.00%.
The diff coverage is 54.54%.

@@           Coverage Diff           @@
##             main    #2374   +/-   ##
=======================================
  Coverage   63.29%   63.29%           
=======================================
  Files         132      132           
  Lines       17245    17256   +11     
=======================================
+ Hits        10916    10923    +7     
- Misses       6329     6333    +4     
Impacted Files Coverage Δ
astroquery/cadc/core.py 76.87% <54.54%> (-0.52%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

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.

This needs a rebase to get rid of the merge commit. I think that there maybe something off with on your fork as the change to setup.cfg keeps coming back.

Also, please add a changelog.


.. doctest-skip::

>>> from astroquery.cadc import Cadc
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to keep a code example in the docs, so I would prefer if it would be fixed rather than just removed.

Copy link
Author

Choose a reason for hiding this comment

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

You suggest I should added back with the doctest-skip?

Copy link
Member

Choose a reason for hiding this comment

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

No, I only set it to skip as the example was broken. (Well, we would need a dummy file anyway, but the problem wasn't with the file itself). So if you suggest to temporarily remove it I'm on board with that one, but for the long term I would find it useful to have a working examples and not just the narrative description of how the file upload should work.

Copy link
Author

Choose a reason for hiding this comment

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

Done. I hope it's the correct way.

astroquery/cadc/core.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@andamian
Copy link
Author

This needs a rebase to get rid of the merge commit. I think that there maybe something off with on your fork as the change to setup.cfg keeps coming back.

Also, please add a changelog.

Do I need a changelog even if the API and functionality are essentially the same (just added an extra attribute with a default to a method)?

@bsipocz
Copy link
Member

bsipocz commented Apr 25, 2022

Do I need a changelog even if the API and functionality are essentially the same (just added an extra attribute with a default to a method)?

OK, it's fine not to add one if you don't expect this to be directly used by users.

@bsipocz bsipocz merged commit 796085e into astropy:main Apr 27, 2022
@bsipocz
Copy link
Member

bsipocz commented Apr 27, 2022

Thanks @andamian!

Version is on pypi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants