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

Create, publish public API documentation on readthedocs.io #98

Merged
merged 62 commits into from
Jul 13, 2020

Conversation

vinceatbluelabs
Copy link
Contributor

@vinceatbluelabs vinceatbluelabs commented Jul 8, 2020

This documents a minimal public API for records-mover at https://records-mover.readthedocs.io/en/publish_docs/

setup.py Outdated Show resolved Hide resolved
351: tests/integration/itest
349: setup.py
320: records_mover/records/schema/field/__init__.py

Reduce total number of bigfiles violations to 1002 or below!
@@ -2,6 +2,8 @@
src="https://raw.githubusercontent.com/bluelabsio/records-mover/master/docs/records-mover-horizontal.png"
alt="Records Mover">

[![Documentation Status](https://readthedocs.org/projects/records-mover/badge/?version=publish_docs)](https://records-mover.readthedocs.io/en/publish_docs/?badge=latest)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Update readthedocs URLs back to master branch

@@ -20,4 +20,4 @@ pip3 install --upgrade pip
# It's nice to unit test, integration test, and run the CLI in
# a development pyenv.
#
pip3 install -r requirements.txt -e '.[unittest,itest,cli,typecheck]'
pip3 install -r requirements.txt -e '.[unittest,itest,cli,typecheck,docs]'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this extra, you can run 'make docs' in the docs/ directory and generate things locally.


* :ref:`genindex`
* :ref:`modindex`
* :ref:`search`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was sort of expecting documentation generation to be entirely driven by docstring comments, but the Sphinx toolkit seems to be a tad more rustic than that. As a result, these .rst files contain:

  • any tweaks that need to be done to the default generation strategy (you'll see some examples later on)
  • lists of the specific public API that will be documented
  • any module-level documentation needed, which doesn't seem to be brought in at all (I generally chose to leave this out and preferred documenting things at the class level)

:maxdepth: 4

records_mover.records.sources
records_mover.records.targets
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the 'documenting only the public API' comes from judicious exclusion of things from these subpackage lists.

<https://github.com/bluelabsio/records-mover/blob/master/docs/RECORDS_SPEC.md>`_
for semantics of each.

alias of Literal[dumb, csv, bigquery, bluelabs, vertica]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, there's no way to assign a docstring to:

DelimitedVariant = Literal[dumb, csv, bigquery, bluelabs, vertica]

...so I had to do it out-of-band here.

.. autoclass:: records_mover.records.base_records_format.BaseRecordsFormat
:members:
:show-inheritance:
:special-members: __init__
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the time I chose to bring up key public API classes into the __all__ of records_mover or record_mover.records, which also ensures that Sphinx will document it there. I then left out the implementation module where it was defined from Sphinx so it didn't get double-documented, and so that we can move things around without folks doing deep 'import' statements.

This is a case where I couldn't get away with that, as Sphinx documented it as a base class of DelimitedRecordsFormat and ParquetRecordsFormat--leaving this out broke the link. I couldn't figure how to convince it to do otherwise.

:exclude-members: Session

.. autoclass:: records_mover.Session
:members: records, set_stream_logging, get_db_engine, get_default_db_engine
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I documented only specific methods here. In particular I didn't yet document the Creds hierarchy - it seemed like a whole different can of worms to lock down at this point. As a result, the methods needed to do google sheets work aren't documented in Sphinx yet - I dropped a post-1.0 backlog item to deal with that.

@@ -1 +1 @@
1002
1019
Copy link
Contributor Author

Choose a reason for hiding this comment

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

setup.py has more extras for documentation generation.

'Records',
'move'
'move',
'MoveResult'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's bring more things so they can be pulled in with a simple import from records_mover.records.

# the first method which makes sense for your new source/target.
# See documentation for RecordsSource in sources.py and
# RecordsTarget in targets.py for the semantics of the methods
# being called.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave the maintenance documentation out of the public API documentation.

'RecordsSchemaFieldConstraints',
'RecordsSchemaFieldStatistics',
'RecordsSchemaFieldRepresentation',
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started down the path of bringing this stuff into the public API but ultimately punted. At least we can have simpler imports of it in the meantime.

RecordsFormatType = Literal['delimited', 'parquet']

VALID_VARIANTS = ['dumb', 'csv', 'bigquery', 'bluelabs', 'vertica']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This thing was only used in unit tests, and can now be done with typing_inspect anyway.

#
# Note: Any expansion of these types should also be done in
# records.jobs.hints
#
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer true! These are all sourced from one place now.

@vinceatbluelabs vinceatbluelabs requested a review from cwegrzyn July 9, 2020 15:23
@vinceatbluelabs vinceatbluelabs merged commit 3312802 into master Jul 13, 2020
@vinceatbluelabs vinceatbluelabs deleted the publish_docs branch July 13, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants