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

Basic sphinx cleanup #32

Merged
merged 10 commits into from
Sep 14, 2016
Merged

Basic sphinx cleanup #32

merged 10 commits into from
Sep 14, 2016

Conversation

chadwhitacre
Copy link
Contributor

No description provided.

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Sep 10, 2016

I ended up putting automodule definitions in relevant docstrings, but there's a wart: I'm not seeing that Sphinx generates the proper structure unless the automodule calls are distributed at a certain level, but putting an automodule line in a docstring for an __init__ means that the subsections come before the docs for anything defined in the __init__. Another reason to do only use __init__ for imports, I suppose.

@chadwhitacre
Copy link
Contributor Author

screen shot 2016-09-10 at 6 02 07 pm

@chadwhitacre
Copy link
Contributor Author

I've switched http://core.aspen.io/en/latest/ to build this branch for now. One thing I'm noticing is that the nav doesn't scroll and is weirdly expanded (all levels are listed flat, and you can expand levels with sublevels). I went looking for a related ticket in the RtD theme repo, and found this one which led me to this project where the nav behaves as I'd expect it to. What's going on, I wonder?

@chadwhitacre
Copy link
Contributor Author

I am daunted by the amount of work to do here. 😩

@chadwhitacre
Copy link
Contributor Author

One thing I'm noticing is that the nav doesn't scroll and is weirdly expanded

The same goofiness is present on http://postgres-py.readthedocs.io/en/2.2.1/.

@chadwhitacre
Copy link
Contributor Author

I'm following these heading styles (I saw those same ones somewhere else, too).

@@ -80,6 +80,7 @@ def __env(envdir):
def env():
"""set up a base virtual environment"""
_env()
_deps()
Copy link
Member

Choose a reason for hiding this comment

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

You should put that in the _sphinx_cmd() function instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? I want the dependencies to be installed when I build the environment, don't I?

Copy link
Member

Choose a reason for hiding this comment

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

No, now that we use tox the env's basic task is only to bootstrap tox if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like I need to bone up on tox ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, so now we have up to four virtualenvs: the env one that we build ourselves, which is only used for tox, and then the .tox/py{27,34,35} ones, which contain the dependencies for the project. If I want to interact with the project directly (run py.test, import aspen), then I need to use one of the tox-managed virtualenvs, not env. 👍

@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Sep 12, 2016

Alright, I think this is good enough to review/merge as a first pass at making docs visible. The next step from here is to start refactoring the codebase to make the overall structure sensible (e.g. #32 (comment)), and then I'll want to go over the whole thing another three or four times. :-)

@chadwhitacre
Copy link
Contributor Author

@Changaco Ready for review! :-)

@Changaco
Copy link
Member

I've just realized that this PR is in fact reverting the reorganization I had done in fc59892 (moving headings from python docstrings to the index.rst file).

@Changaco
Copy link
Member

I've imported commits from AspenWeb/pando.py#572.

, install_requires = ASPEN_DEPS
, tests_require = TEST_DEPS
, install_requires = open('requirements.txt').read()
, tests_require = open('requirements_tests.txt').read()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really sufficient? We don't need a splitlines here?

Copy link
Member

@Changaco Changaco Sep 14, 2016

Choose a reason for hiding this comment

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

No splitlines, it would probably fail on empty lines and comments if we naively split the lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The simplest way to include requirement specifiers is to use the install_requires argument to setup(). It takes a string or list of strings containing requirement specifiers. If you include more than one requirement in a string, each requirement must begin on a new line.

TIL.

@chadwhitacre
Copy link
Contributor Author

I've just realized that this PR is in fact reverting the reorganization I had done in fc59892 (moving headings from python docstrings to the index.rst file).

What was the intent behind the reorganization? I don't see on #1 that we talked about it at the time (we had other things to talk about ;).

@Changaco
Copy link
Member

What was the intent behind the reorganization?

I remember thinking that it seemed weird to put the header in each file, and cleaner to put it in the .rst file. If there was another reason, I don't remember it.

@chadwhitacre
Copy link
Contributor Author

I agree it's weird if it's only the header in the docstring. I see them as placeholders, though. I expect to write more docs in the docstrings. Let's see if it still looks weird after that ...

envdir = _env()
run('pip', 'install', *packages)
_deps()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@chadwhitacre chadwhitacre Sep 14, 2016

Choose a reason for hiding this comment

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

Harumph. If tox supported multiple requirements files there may have been something here. I'm not going to pursue this further for now, though.

@Changaco
Copy link
Member

The merge is yours, I was the last one to add commits. ;-)

@chadwhitacre
Copy link
Contributor Author

Okay! I'm good with your commits. Ready to merge? :)

@chadwhitacre
Copy link
Contributor Author

Heh. Crossed signals. :)

@chadwhitacre chadwhitacre merged commit f771204 into master Sep 14, 2016
This was referenced Sep 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants