Skip to content

A number of CADC authentication and doc fixes#2374

Merged
bsipocz merged 9 commits into
astropy:mainfrom
andamian:cadc_datalink
Apr 27, 2022
Merged

A number of CADC authentication and doc fixes#2374
bsipocz merged 9 commits into
astropy:mainfrom
andamian:cadc_datalink

Conversation

@andamian

@andamian andamian commented Apr 25, 2022

Copy link
Copy Markdown

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

codecov Bot commented Apr 25, 2022

Copy link
Copy Markdown

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

@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.

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.

Comment thread docs/cadc/cadc.rst

.. doctest-skip::

>>> from astroquery.cadc import Cadc

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.

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

Comment thread astroquery/cadc/core.py Outdated
Comment thread setup.cfg Outdated
@andamian

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

bsipocz commented Apr 25, 2022

Copy link
Copy Markdown
Member

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

bsipocz commented Apr 27, 2022

Copy link
Copy Markdown
Member

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.

2 participants