astropy / pyregion

ds9 region parser for python
https://pyregion.readthedocs.io
MIT License
39 stars 38 forks source link

Update to astropy.visualization.wcsaxes #117

Closed cdeil closed 6 years ago

cdeil commented 6 years ago

This PR implements #116, i.e. updates from standalone wcsaxes to astropy.visualization.wcsaxes in the pyregion package.

@astrofrog - Is this update OK or can it be done somehow in a better way?

All- The statement in the install docs is that pyregion still runs on Astropy >=1 although I'm not sure if it's true. So I don't know if backward compat is an issue here or how far back support for old Astropy versions is desired here. Maybe it would be OK to bump the required version to Astropy >= 1.3?

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 63.295% when pulling 1d1409acb4e0be2854e4c43d1e514f9cadd4f898 on cdeil:wcsaxes-update into 12fc5d8d192ae3027bd705ccc434ba0d80730e25 on astropy:master.

cdeil commented 6 years ago

@bsipocz - Do you know how to fix this Sphinx warning?

/home/travis/build/astropy/pyregion/docs/api/pyregion.ShapeList.rst:7: WARNING: py:class reference target not found: list
/home/travis/build/astropy/pyregion/docs/getting_started.rst:34: WARNING: py:obj reference target not found: list

https://travis-ci.org/astropy/pyregion/jobs/251688184#L673

bsipocz commented 6 years ago

Hmm, it seems that they for some reason removed list() for the intersphinx inventory for python2. There isn't even an anchor on it https://docs.python.org/2.7/library/functions.html#len

So there are 3 options. Easiest it to use double backticks and try not to link; or build the docs only on python3; or add list to the local instersphinx file that is shipped with astropy-helpers (though given that there doesn't seem to be a python2 linkable one, that link should always point to python3).

cdeil commented 6 years ago

Hmm, it seems that they for some reason removed list() for the intersphinx inventory for python2. There isn't even an anchor on it https://docs.python.org/2.7/library/functions.html#len

That seems very weird that they'd remove it now after 10+ years. I'll see if I can find the right issue tracker to report something like this with Python core.


For now, to get green light on CI, I wanted to use double backticks and had a look. But the first warning is due to the list when sub-classing, i.e. I can't add backticks there:

class ShapeList(list)

So maybe try the intersphinx suggestion you pointed out?

Is it this file?

astropy_helpers/astropy_helpers/sphinx/local/python2_local_links.txt

@bsipocz What line exactly do we have to add? Do we have to run a command to update the .inv file? Could you please add a commit to this PR doing it?

cdeil commented 6 years ago

I've complained about the missing list in the Python 2.7 docs here: http://bugs.python.org/issue30882

Possibly we'll just have to wait a bit until they add it back and take no action here.

@astrofrog - Apart from this unrelated issue, what do you think about this PR (see original description at the top)?

bsipocz commented 6 years ago

@cdeil - Thanks for reporting it upstream. Hopefully they'll move to GH soon, so I'll start reporting these issues, currently their UI makes me prefer living with the issues rather than opening a ticket for them.

cdeil commented 6 years ago

Hopefully they'll move to GH soon, so I'll start reporting these issues,

I vaguely remember some talk or blog post where they described their move to Github for development, and the reasons they won't move the issue tracker to Github any time soon.

currently their UI makes me prefer living with the issues rather than opening a ticket for them.

Agreed, the web interface is ugly. But it works, took me 2 min to file the issue.

bsipocz commented 6 years ago

I go ahead and merge this as only the upstream list related issue remained. I can also update the travis matrix once there is an agreement on which astropy version should we support. I think 1.0.x is an overkill, but probably still doable, so I'm quite neutral on the question.