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.

Sign up for GitHub

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

Jump to bottom

Document how to do local development #162

Merged
merged 5 commits into from
Jan 6, 2021
Merged

Document how to do local development #162

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
67e963c
Document how to do local development
yuvipanda Dec 14, 2020
7c22dd1
Move 'contributing' section after 'installation'
yuvipanda Dec 15, 2020
61c3603
Add section on git cloning
yuvipanda Dec 15, 2020
5d443c6
Remove unnecessary myst config
yuvipanda Jan 5, 2021
ef4a12b
Remove extreneous packages from doc-requirements
yuvipanda Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 6 additions & 15 deletions docs/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,11 @@

import os
import requests
from recommonmark.transform import AutoStructify

github_doc_root = "https://github.com/rtfd/recommonmark/tree/master/doc/"


def setup(app):
app.add_config_value(
"recommonmark_config",
{
"url_resolver": lambda url: github_doc_root + url,
"auto_toc_tree_section": "Contents",
},
True,
)
app.add_transform(AutoStructify)
app.add_stylesheet("custom.css")
app.add_javascript("link_gen/link.js")

Expand All @@ -31,7 +21,12 @@ def setup(app):
# Add any Sphinx extension module names here, as strings. They can be
# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
# ones.
extensions = ["sphinx_copybutton"]
extensions = [
"myst_parser",
"sphinx.ext.intersphinx",
]

myst_admonition_enable = True
Copy link
Member

Choose a reason for hiding this comment

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

I believe this doesn't need to be enabled explicitly as I use ```{admonition} ...``` directives already in z2jh's docs.

Copy link
Member

Choose a reason for hiding this comment

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

correct, I believe (also, in the new myst-parser this configuration will be deprecated in favor of a list of extensions, so let's try just removing it for now?)

Copy link
Member

@consideRatio consideRatio Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah yes, this is how I enable the substitution extension in z2jh currently.

# myst_enable_extensions ref: https://myst-parser.readthedocs.io/en/latest/using/syntax-optional.html
myst_enable_extensions = [
    "substitution",
]

myst_substitutions = {
    "latest_tag": latest_tag,
    "chart_version": chart_version,
    "chart_version_git_ref": chart_version_git_ref,
    "jupyterhub_version": jupyterhub_version,
    "kube_version": kube_version,
    "requirements": f"[hub/images/requirements.txt](https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/{chart_version_git_ref}/images/hub/requirements.txt)",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed! I don't even know what admonitions are :D

Copy link
Member

@consideRatio consideRatio Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

A more general class for note, important, warning for example. A benefit is that you can give them a title as well, here is how!

```{admonition} Title of a note
:class: note

This is the content of my note
```

Here is an example of a :class: warning admonition with a custom title from the z2jh guide.

image

Copy link
Member

Choose a reason for hiding this comment

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

Removed! I don't even know what admonitions are :D

It seems to remain in the PR still, I think a cat must have disrupted the push :)


# Add any paths that contain templates here, relative to this directory.
templates_path = ["_templates"]
Expand All @@ -41,10 +36,6 @@ def setup(app):

source_suffix = [".rst", ".md"]

from recommonmark.parser import CommonMarkParser

source_parsers = {".md": CommonMarkParser}


# The master toctree document.
master_doc = "index"
Expand Down
42 changes: 42 additions & 0 deletions docs/contributing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Contributing

## Setup

nbgitpuller is a [jupyter
serverextension](https://jupyter-notebook.readthedocs.io/en/stable/extending/handlers.html),
and hence can be developed locally without needing a JupyterHub.

1. Setup a virtual environment to do development in

```bash
python3 -m venv venv
source venv/bin/activate
```

2. Install nbgitpuller with its dependencies in this virtual environment

```bash
pip install -e .
yuvipanda marked this conversation as resolved.
Show resolved Hide resolved
```

3. Enable the nbgitpuller jupyter serverextension

```bash
jupyter serverextension enable --sys-prefix nbgitpuller
```

4. Start the notebook server. This will open the classic notebook in your web
browser, and automatically authenticate you as a side effect.

```bash
jupyter notebook
```

5. You can now test nbgitpuller locally, by hitting the `/git-pull` url with any
of the [URL query parameters](topic/url-options.rst). For example, to pull the
[data-8/textbook](https://github.com/data-8/textbook) repository's `gh-pages`
branch, you can use the following URL:

```
http://localhost:8888/git-sync?repo=https://github.com/data-8/textbook&branch=gh-pages
```
2 changes: 1 addition & 1 deletion docs/doc-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
recommonmark==0.4.0
sphinx_copybutton
traitlets
jupyterhub
Expand All @@ -7,3 +6,4 @@ sphinx-book-theme
memory_profiler
pytest
PyGitHub
myst_parser[sphinx]
Copy link
Member

Choose a reason for hiding this comment

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

From experience, order matters so I have the suggestion of mimicing the order in z2jh to avoid potential issues with RTD that has sphinx pre-installed already. To conclude, I think the key point is to let myst_parser be at the top, and that [sphinx] isn't needed, sphinx is assumed by default as a requirement.

Relevant configs from z2jh for reference.

doc-requirements.txt

# Order matters:
# - myst-parser depends on "sphinx>=2,<4"
# - pydata-sphinx-theme depends on "sphinx"
# - sphinx-copybutton depends on "sphinx>=1.8"
#
# Listing either pydata-sphinx-theme or sphinx-copybutton first will make the
# myst-parser constraints on sphinx be ignored, so myst-parser should go
# first. This is only relevant if sphinx==1.* is already installed in the
# environment, which it is on ReadTheDocs.
#
chartpress
myst-parser
pydata-sphinx-theme
pyyaml
sphinx-autobuild
sphinx-copybutton
sphinxext-rediraffe

.readthedocs.yml

# Configuration on how ReadTheDocs (RTD) builds our documentation
# ref: https://readthedocs.org/projects/zero-to-jupyterhub/
# ref: https://docs.readthedocs.io/en/stable/config-file/v2.html

# Required (RTD configuration version)
version: 2

# Build documentation in the docs/ directory with Sphinx
sphinx:
  configuration: doc/source/conf.py

# Optionally build your docs in additional formats such as PDF and ePub
formats: []

# Optionally set the version of Python and requirements required to build your docs
python:
  version: 3.7
  install:
    # WARNING: This requirements file will be installed without the pip
    #          --upgrade flag in an existing environment. This means that if a
    #          package is specified without a lower boundary, we may end up
    #          accepting the existing version.
    #
    #          ref: https://github.com/readthedocs/readthedocs.org/blob/0e3df509e7810e46603be47d268273c596e68455/readthedocs/doc_builder/python_environments.py#L335-L344
    - requirements: doc/doc-requirements.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but this doesn't use readthedocs for docs - it's hosted on GitHub pages i believe.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know exactly all details, but I suspect that if sphinx 4 is released, it will be installed and break things with this ordering. To safeguard for this, could you...
1: move myst_parser to the top
2: remove dedicated line of installing sphinx

I was about to push a commit and merge but I realized that the admonition thingy was still here and you probably have unpushed commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to understand - https://github.com/jupyterhub/nbgitpuller/blob/master/.circleci/config.yml is the current config used to build and deploy the docs. Should sphinx still be removed?

Copy link
Member

Choose a reason for hiding this comment

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

In doc-requirements.txt, i think letting myst-parser install as the first item can help avoid a potential issue i struggled with that may show up if for example sphinx 4 is released.

Removing sphinx is also fine as it is an myst_parser install requirement, but not important as long it is listed after myst_parser.

1 change: 1 addition & 0 deletions docs/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ Full Contents
.. toctree::
:maxdepth: 2

contributing
yuvipanda marked this conversation as resolved.
Show resolved Hide resolved
install
topic/automatic-merging
topic/url-options
Expand Down