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 defaults for cookiecutter'd project #2555

Merged
merged 7 commits into from
Jul 4, 2020

Conversation

hanhanhan
Copy link
Contributor

@hanhanhan hanhanhan commented Apr 21, 2020

Description

Add sphinx defaults for cookiecutter'd project

  • serve, watch + live reload for docs + code file changes
  • separate _source and _build
  • add packages and paths to use autodoc
  • edit/add documentation with examples (both at django-cookiecutter and inside generated project)
  • add formatted comments in User model for pickup by Sphinx apidoc - fixes Set up auto-generated Sphinx API docs #526
  • serve docs from a separate docs container for docker build

Rationale

Setting up sphinx documentation is fiddly (and often low priority).

How about a setup that works well from the start?
If it's easier to document as you go, it's more likely to happen.

Use case(s) / visualization(s)

Try:

make livehtml

locally in the docs folder, or in the docs container.

Navigate to port 7000 on your localhost/host (this is in the django-cookiecutter docs now too).

Considerations

  • The make.bat file needs testing. I will. I tested the make.bat file
  • You may want to structure the docker config differently. To me, it was a tradeoff between using the django base image and inheriting postgres dependencies, and just creating another container. Or serving the docs from the django container, which seemed messy. Maybe you, reviewer person, have a good idea to try instead?


3. Run the following command: ::

$ sphinx-apidoc -f -o ./docs/modules/ ./tpub/ migrations/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sphinx autodoc is cool, but apidoc seems less useful. So it's there but I don't talk about it.

SOURCEDIR = .
BUILDDIR = _build
SPHINXOPTS ?=
SPHINXBUILD ?= sphinx-build -c .
Copy link
Contributor Author

@hanhanhan hanhanhan Apr 21, 2020

Choose a reason for hiding this comment

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

-c . flag used because conf.py is in different location from source files

Comment on lines +8 to +20
"""Default user for {{cookiecutter.project_name}}.
"""

# First Name and Last Name do not cover name patterns
# around the globe.
#: First and last name do not cover name patterns around the globe
name = CharField(_("Name of User"), blank=True, max_length=255)

def get_absolute_url(self):
"""Get url for user's detail view.

Returns:
str: URL for user detail.

"""
Copy link
Contributor Author

@hanhanhan hanhanhan Apr 21, 2020

Choose a reason for hiding this comment

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

Use google style docstring, picked up by apidoc, as an example.
The comments aren't that illuminating, but it shows how this works.

@hanhanhan hanhanhan force-pushed the docs branch 6 times, most recently from 09b6601 to 758d66e Compare April 27, 2020 16:23
@browniebroke
Copy link
Member

The diff is now showing the things which have been merged recently, something probably gone wrong in a rebase. Could you try tidying this up?

I don't mind merge commits, and they are usually safer, now it's a bit tricky to review.

@hanhanhan
Copy link
Contributor Author

Yes, I was rebasing as I made some fixes but my commit got buried. I'll drop the non-PR commits.

-serve, watch + live reload for docs + code file changes
-update project makefile + make.bat
-separate _source and _build
-add packages and paths to use autodoc
-edit/add documentation with examples (both at django-cookiecutter and inside generated project)
-add formatted comments in User model for pickup by Sphinx apidoc
-serve docs from a separate docs container for docker build

# Catch-all target: route all unknown targets to Sphinx using the new
# "make mode" option. $(O) is meant as a shortcut for $(SPHINXOPTS).
%: Makefile
@$(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
@$(SPHINXBUILD) -b $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Sphinx only recognizes the -M option if it is placed first."

Include as helper comment only
@hanhanhan
Copy link
Contributor Author

I wish I had used type hints instead of the older style docstrings, but I'd rather just get this closed and (hopefully) come back later than start making more edits.

@hanhanhan hanhanhan closed this May 18, 2020
@hanhanhan
Copy link
Contributor Author

hanhanhan commented May 18, 2020

Whoops! I accidentally closed this instead of hitting "comment"!

@hanhanhan hanhanhan reopened this May 18, 2020
@wadkar
Copy link
Contributor

wadkar commented May 18, 2020

Whoops! I accidentally closed this instead of hitting "comment"!

Happens to the best of us, no worries 😄

@hanhanhan
Copy link
Contributor Author

I made a "resolve conflicts" that introduced a typo, hence failing tests.
So I just dropped that github-only commit.

Is there anything else I should do to get this ready to merge?

@browniebroke
Copy link
Member

Is there anything else I should do to get this ready to merge?

Not really at this point. I need to find a bit of time to sit down and review this in properly. Don't worry too much about the conflicts for now, let's do it just before merging (I can do it if needed).

Comment on lines 15 to 17
&& apt-get install -y texlive-fonts-recommended \
&& apt-get install -y texlive-latex-extra \
&& apt-get install -y latexmk \
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these if we don't use latex?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it was decided to drop it.

Copy link
Contributor Author

@hanhanhan hanhanhan Jun 27, 2020

Choose a reason for hiding this comment

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

Edit: I just removed this in an additional fixup commit. Do you prefer I squash those? Does it matter?

I commented line 14 -- I meant to comment out lines 14-17 with the note on line 13.
I've seen little notes like that in the project elsewhere.
Should I just delete instead?
I'll fix it in either direction.

Copy link
Member

Choose a reason for hiding this comment

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

Don't worry about squashing, we can do it at merge time.

I don't mind either way, really. I don't use latex anymore, so I'd be in favour of removing them, but that's subjective.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @hanhanhan question was whether to keep the lines but comment them out. Personally, I rather delete lines then comment them because, well - that's what git is for! 😄

Copy link
Member

Choose a reason for hiding this comment

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

Git history won't be preserved into projects that will be generated, only in the template code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point - didn't think it from that perspective. This is super helpful PR and I tried out on a demo project recently and I can report that it worked fine with few custom modifications I need for my local docker setup (not relevant to the project).

Thanks a lot @hanhanhan !

hanhanhan and others added 5 commits June 27, 2020 13:57
sounds good!

Co-authored-by: Bruno Alla <browniebroke@users.noreply.github.com>
Co-authored-by: Bruno Alla <browniebroke@users.noreply.github.com>
# Conflicts:
#	{{cookiecutter.project_slug}}/requirements/local.txt
Copy link
Member

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

Gave it a final look, and it looks good.

@browniebroke browniebroke merged commit 19c151b into cookiecutter:master Jul 4, 2020
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.

Set up auto-generated Sphinx API docs
4 participants