improve target coordinate specification in jplhorizons

[m-stclair]

closes #1555
Fixes #2599.

[m-stclair]

hi @bsipocz -- I've reimplemented these features, added basic test coverage for them, updated other tests as necessary, and updated docstrings. I'm assuming the fast CI failures where the pipeline can't find tox are because this is a draft; if not, I have no idea how to interrogate those. What else do you think needs to be done before I mark this as ready for review?

[bsipocz]

It's a weird and new failure about tox, the draft status of the PR shouldn't affect the CI this way. I'll look into it and fix it separately.

If the content is ready, then do mark it ready for review, and we can ask @mkelley to do a first round of review while I'm dealing with CI.
(Tests can be checked locally, including the remote tests, etc.)

[bsipocz]

@m-stclair - could you rebase? CI has been fixed, but I cannot step in as a maintainer to do the rebase as the PR was opened from an org fork rather than from yours.

[m-stclair]

Ty for fixing the tox issue. Tests pass locally. I have rebased and am now marking this ready for review.

[codecov]

Codecov Report

Merging #2625 (671f8a3) into main (a867890) will decrease coverage by 0.37%.
The diff coverage is 58.53%.

❗ Current head 671f8a3 differs from pull request most recent head 5152eed. Consider uploading reports for the commit 5152eed to get more accurate results

@@            Coverage Diff             @@
##             main    #2625      +/-   ##
==========================================
- Coverage   64.33%   63.96%   -0.38%     
==========================================
  Files         130      130              
  Lines       16836    16870      +34     
==========================================
- Hits        10832    10791      -41     
- Misses       6004     6079      +75     

Impacted Files	Coverage Δ
astroquery/jplhorizons/core.py	65.40% <58.53%> (-0.22%)	⬇️
astroquery/cds/core.py	22.91% <0.00%> (-45.84%)	⬇️
astroquery/heasarc/core.py	75.48% <0.00%> (+1.69%)	⬆️

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

[m-stclair]

The codecov misses are pretty small things, but let me know if it's important to cover them and I'll add a couple of little unit tests!

[bsipocz]

Some minor nitpicks, and please add a changelog entry.

Also, please update the narrative docs, changes here have effect on some of the doctest expectations there.

[m-stclair]

will add a changelog entry and update narrative docs tomorrow or tuesday.

[bsipocz]

The codecov misses are pretty small things, but let me know if it's important to cover them and I'll add a couple of little unit tests!

codecov is red herring here, it doesn't look for the changes with the remote tests, so it misses the actual ~2% test increase

[m-stclair]

on codecov: got it, makes sense

[m-stclair]

@bsipocz , I believe I've addressed all your requests.

[m-stclair]

@eerovaher , I believe I've addressed all your suggestions. I do not understand why the readthedocs build is now failing, but it does not appear to be related to any actual code changes.

[eerovaher]

I don't really understand why the documentation build failed. Perhaps if you squash some of the commits (not necessarily down to one) and then force-push to GitHub the next documentation build might succeed.

[bsipocz]

I do not understand why the readthedocs build is now failing, but it does not appear to be related to any actual code changes.

This is a weird problem, I don't think I saw it before (the commit hashes match, yet RTD complains that it cannot check it out). A restart didn't resolve it, so I would say wait it out while I investigate whether other repos are affected, too, or try with the squashing idea.

[bsipocz]

OK, leave the RTD for now, this is a more widespread problem and not much we can do about it in this PR.

[earthshrink]

hi @m-stclair, in first comment above, did you mean closes #2599?

[m-stclair]

No, it's just the confusing GitHub convention where PRs and issues are notated the same way, and this is PR 2625. When I opened this PR I hadn't noticed you'd opened that issue. However, it's definitely true that this closes #2599. Editing first message to add that.

[bsipocz]

Sorry, that was my bad, I indeed meant to add #2599 there and not the PR's number. Thanks for spotting it. 😄

[m-stclair]

Nevermind then! I thought it had been automatically added.

…

On Thu, Dec 15, 2022 at 5:46 PM Brigitta Sipőcz ***@***.***> wrote: Sorry, that was my bad, I indeed meant to add #2599 <#2599> there and not the PR's number. Thanks for spotting it. 😄 — Reply to this email directly, view it on GitHub <#2625 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APIXBZIVPJAPTAGVI3XA5MTWNNKOJANCNFSM6AAAAAAS2QP3O4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bsipocz]

Yes, your original PR (#1929) predates all the new issues. And based on that, this would also address #1555

[bsipocz]

Thanks @m-stclair!