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

Document how to do local development #162

Merged
merged 5 commits into from Jan 6, 2021

Conversation

yuvipanda
Copy link
Contributor

  • Move from recommonmark to MyST while we're at it.

- Move from recommonmark to MyST while we're at it.
@yuvipanda
Copy link
Contributor Author

/cc @trevwilliams107 and @chrispyles. Would love for you to try it out and let me know if this works.

docs/index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@ryanlovett ryanlovett left a comment

Choose a reason for hiding this comment

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

Looks good to me!

docs/contributing.md Show resolved Hide resolved
@yuvipanda
Copy link
Contributor Author

Anyone wanna merge this? :)

@yuvipanda
Copy link
Contributor Author

3

@yuvipanda yuvipanda closed this Jan 5, 2021
@choldgraf
Copy link
Member

Accidental close?

docs/conf.py Outdated
"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

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

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 :)

@@ -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.

@yuvipanda yuvipanda reopened this Jan 5, 2021
@yuvipanda
Copy link
Contributor Author

Definitely accidental!

@yuvipanda
Copy link
Contributor Author

3t

@yuvipanda yuvipanda closed this Jan 5, 2021
@manics
Copy link
Member

manics commented Jan 5, 2021

and again? 😸

@yuvipanda yuvipanda reopened this Jan 5, 2021
@yuvipanda
Copy link
Contributor Author

it is, indeed, a mystery. What do those numbers mean?!

@manics
Copy link
Member

manics commented Jan 5, 2021

Are you using the GitHub app on your phone/tablet? Is it unlocked in your pocket and detecting touches? Do you have a cat lounging on your keyboard?

@consideRatio
Copy link
Member

Do you have a cat lounging on your keyboard?

Really excited about the possibility of seeing some cat contributions! 😍 Mentor the cat!!!

yuvipanda added a commit to yuvipanda/nbgitpuller that referenced this pull request Jan 6, 2021
@yuvipanda
Copy link
Contributor Author

Ah, some git fuckery had happened. Has been fixed now, and I think it addresses all the review comments.

@consideRatio consideRatio merged commit d7bdacf into jupyterhub:master Jan 6, 2021
@consideRatio
Copy link
Member

🥳

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.

None yet

5 participants