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

Add Sphinx automod API docs #83

Merged
merged 7 commits into from
Aug 2, 2016
Merged

Add Sphinx automod API docs #83

merged 7 commits into from
Aug 2, 2016

Conversation

cdeil
Copy link
Member

@cdeil cdeil commented Aug 2, 2016

This PR implements #53 and adds Sphinx automod API docs for pyregion.

It currently looks like this:

screen shot 2016-08-02 at 09 26 07

I'll keep working on this a little (cross-link getting started and API docs), but then merge it so that review / feedback becomes easier.

The main questions are which other functions / classes should be part of the public API and appear here.

@cdeil cdeil added this to the v1.2 milestone Aug 2, 2016
@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.03%) to 50.689% when pulling 1a216df on cdeil:api-docs into d9133d6 on astropy:master.

header : `~astropy.io.fits.Header`
FITS header
rot_srt_axis : bool
TODO: document me
Copy link
Member Author

Choose a reason for hiding this comment

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

@leejjoon - It looks like rot_wrt_axis is used as a bool flag? OK if I change the default values of 1 / 0 to True / False everywhere? What should be the parameter description (one or two lines)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it was meant to be 1 or 2 according to the original PR (#34) by @mcara. Although in the code, it checks whether it is 1 or not.

Copy link
Contributor

@mcara mcara Aug 2, 2016

Choose a reason for hiding this comment

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

@leejjoon said it all (thanks!)

The intention for this parameter was to indicate the axis relative to which the rotation angle is specified (when CD/PC matrix contains skews). Therefore, it should be a number (an integer) rather than a boolean.

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage decreased (-0.0007%) to 50.662% when pulling 2629ac5 on cdeil:api-docs into d9133d6 on astropy:master.

*pyregion.open* takes the region name as an argument and returns a
ShapeList object, which is basically a list of Shape objects. ::
`pyregion.open` takes the region name as an argument and returns a
`~pyregion.ShapeList` object, which is basically a list of Shape objects. ::
Copy link
Member Author

Choose a reason for hiding this comment

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

@leejjoon - The Shape class should be part of the HTML API docs, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do make it part of the API docs, should it be exposed as pyregion.parser_helper.Shape like it is now, or re-exposed from pyregion.Shape?

@cdeil cdeil self-assigned this Aug 2, 2016
@cdeil
Copy link
Member Author

cdeil commented Aug 2, 2016

I'm merging this now.

Of course, comments and PRs are still welcome if you only see this later!

I noticed a few issues where cleanup would imply slight API changes (e.g. changing the name of arguments from s to something more obvious). I'll split those out into separate issues and PRs so that @leejjoon and others can comment whether they are OK changes for 1.2 or whether they should be avoided completely or delayed to a possible 2.0 in the future for better backward compatibility.

@cdeil cdeil merged commit 1304632 into astropy:master Aug 2, 2016
@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.05%) to 50.714% when pulling 40805a1 on cdeil:api-docs into d9133d6 on astropy:master.

@cdeil
Copy link
Member Author

cdeil commented Aug 2, 2016

Current API docs are here:
http://pyregion.readthedocs.io/en/latest/api.html

Everyone: please review and make PRs or issues for corrections or omissions!

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.

4 participants