Thanks! Need to add test(s) and a change log.

Codecov Report

Merging #8767 into master will decrease coverage by <.01%.
The diff coverage is 12.5%.

@@            Coverage Diff             @@
##           master    #8767      +/-   ##
==========================================
- Coverage   86.96%   86.95%   -0.01%     
==========================================
  Files         400      400              
  Lines       59448    59451       +3     
  Branches     1100     1100              
==========================================
+ Hits        51697    51698       +1     
- Misses       7110     7112       +2     
  Partials      641      641

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 660d73b...f8045b3. Read the comment docs.

Codecov Report

Merging #8767 into master will increase coverage by 0.01%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #8767      +/-   ##
==========================================
+ Coverage   86.96%   86.97%   +0.01%     
==========================================
  Files         400      400              
  Lines       59454    59456       +2     
  Branches     1100     1100              
==========================================
+ Hits        51703    51714      +11     
+ Misses       7110     7101       -9     
  Partials      641      641

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 1658b5a...6c039c0. Read the comment docs.

Also before merge, the commits need to be squashed to get rid of the "merge commit" message. Please do not use git pull, but instead use git fetch followed by git rebase. Thanks!

Also before merge, the commits need to be squashed to get rid of the "merge commit" message. Please do not use git pull, but instead use git fetch followed by git rebase. Thanks!

I must admit I have no idea about how to do this... Running those two commands doesn't seem to change anything...???

@rhandberg , I apologize for the confusion. Full commands are documented at http://docs.astropy.org/en/latest/development/workflow/development_workflow.html#how-to-rebase

@rhandberg - once you have done the merge commit, the rebase keeps it in place, but one could get rid of it by doing git rebase -i ... and then explicitly removing it. It is no big deal, though!

@rhandberg - the actual PR looks good! But we do need a test! (in coordinates/tests/test_solar_system.py) - this is why codecov complains...

Ideally, the test would use a tiny ephemeris file, but I don't know if such a thing exists (or is easy to create). Alternatively, since we download de432s anyway for the tests, perhaps there is a way to get the path to where it is downloaded (@pllim, would you know how?) and pass that in. If you then also add a test that tries to open just some random astropy file, you can check that with your PR this gives an error from SPK (rather than a URL parse error).

Easier, but no less importantly, could you add an entry in CHANGES.rst under "New Features" for astropy.coordinates? That will make astro-bot happy.

I wondered about adding something to the general documentation, but think that is not necessary: people should just look at the docstring you already changed. That said, for reasons I'm not sure about, docs/coordinates/solarsystem.rst doesn't properly link to solar_system_ephemeris - feel free to fix that! (but you don't have to.)

get the path

Would astropy.utils.data.get_cached_urls help? Or theoretically astropy.utils.data.download_file will grab the cached version, if available.

So, to clarify further, theoretically, you could do this (unless pytest does weird things to caches):

>>> from astropy.utils.data import download_file, get_cached_urls
>>> # The URL is just example, replace with the URL you desire
>>> get_cached_urls()
['http://data.astropy.org/l1448/l1448_13co.fits', ...]
>>> # No actual download would happen in this line below
>>> file_path = download_file('http://data.astropy.org/l1448/l1448_13co.fits', cache=True)
>>> file_path
'C:\\Users\\...\\.astropy\\cache\\download\\py3\\somehashvalue'

What do you think of the following test? It should test the loading of ephemeris using URLs and file paths and that they give the same result.

@pytest.mark.remote_data
@pytest.mark.skipif('not HAS_JPLEPHEM')
@pytest.mark.parametrize('time', (Time('1960-01-12 00:00'),
                                  Time('1980-03-25 00:00'),
                                  Time('2010-10-13 00:00')))
def test_url_or_file_ephemeris(time):
	# URL for ephemeris de432s used for testing:
    url = 'http://naif.jpl.nasa.gov/pub/naif/generic_kernels/spk/planets/de432s.bsp'

	# Pass the ephemeris directly as a URL.
    coord_by_url = get_body('earth', time, ephemeris=url)

    # Translate the URL to the cached location on the filesystem.
    # Since we just used the url above, it should already have been downloaded.
    filepath = download_file(url, cache=True)

	# Get the coordinates using the file path directly:
    coord_by_filepath = get_body('earth', time, ephemeris=filepath)

	# Using the URL or filepath should give exactly the same results:
    assert_quantity_allclose(coord_by_url.ra, coord_by_filepath.ra)
    assert_quantity_allclose(coord_by_url.dec, coord_by_filepath.dec)
    assert_quantity_allclose(coord_by_url.distance, coord_by_filepath.distance)

Maybe a separate test is needed to check that it throws and error if you provide a non-existing file or URL?

@pllim - thanks, that's just the thing! @rhandberg - test looks excellent! And agreed that a test with a non-existing file would be good to add - given your code, this will also check the bad url path (which of course we know works from your initial issue!). Maybe also a test with an existing file that is not an ephemeris file.

Great! What do you think about this for the second test where we are testing for non-existing URL and file path as well as a file that is not an ephemeris file... Actually I'm just passing the current Python file...

@pytest.mark.remote_data
@pytest.mark.skipif('not HAS_JPLEPHEM')
def test_url_or_file_ephemeris_wrong_input():

	time = Time('1960-01-12 00:00')

	# Try loading a non-existing URL:
	with pytest.raises(HTTPError):
		get_body('earth', time, ephemeris='http://data.astropy.org/path/to/nonexisting/file.bsp')

	# Try loading a non-existing file:
	with pytest.raises(ValueError):
		get_body('earth', time, ephemeris='/path/to/nonexisting/file.bsp')

	# Try loading a file that does exist, but is not an ephemeris file:
	with pytest.raises(ValueError):
		get_body('earth', time, ephemeris=__file__)

FWIW

	# Try loading a non-existing URL:
	with pytest.raises(HTTPError):
		get_body('earth', time, ephemeris='http://data.astropy.org/path/to/nonexisting/file.bsp')

can be in its own test function decorated with remote_data. That way, the other two tests can run even when remote_data is not specified (and not needed).

@pllim - Very good point! I will split that test into two.

Tests have now been added, but codecov seems to still fail... Looks like it is because the whole solar_system.py file (not just from this PR) is not covered as well as the rest of astropy

@mhvk - Good catches! Those two issues have been fixed now.