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

Initial implementation #1

Merged
merged 16 commits into from Mar 18, 2022
Merged

Initial implementation #1

merged 16 commits into from Mar 18, 2022

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Feb 23, 2022

The extension itself is pretty simple, most of the files are docs and tests.

I'm not 100% sure about the extension name/settings.

@stsewd stsewd force-pushed the init branch 11 times, most recently from 1614052 to 7fd8d68 Compare February 24, 2022 15:15
@stsewd stsewd marked this pull request as ready for review February 24, 2022 15:37
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Very thorough, I appreciate the addition of some test cases on this code 👍

Major points I raised were the naming/description of the package and some of the concepts. I also hadn't thought much about the functionality to assist conditionally setting configuration values. This seems helpful, but it also seems like something we could derive from the existing subprojects instead of redefining.


python:
install:
- requirements: docs/requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

For the future, I'd suggest trying other RTD configuration patterns than requirements.txt. This is a rather crusty old pattern, and new projects give us a chance to be experimenting new packaging features from the user perspective.

Copy link
Member Author

Choose a reason for hiding this comment

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

do you mean using extras_require? pip install foo[docs]?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good pattern to test, but really any other pattern is helpful to use to get some different perspective. I've used poetry on a few repos too. It's not important for this immediate PR, and we can always play around with things at a later time. I just like to advocate against requirements.txt though, it's sort of a last ditch method for dependencies these days.

.circleci/config.yml Show resolved Hide resolved
README.md Outdated
Comment on lines 6 to 8
Share a single `conf.py` file between several Sphinx projects.
useful if you want to import several projects from the same
repository on [Read the Docs](https://readthedocs.org).
Copy link
Contributor

Choose a reason for hiding this comment

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

So, regarding the name of this repository and how we are describing it's function, "shared configuration" describes how this extension technically works, but it might make the most sense describing why users would use it instead. Users not familiar with this problem/solution probably won't understand why sharing a configuration file is important, or derive some other use case from "sharing a configuration file".

Users hitting this are probably finding it by googling something like "sphinx read the docs mulitple projects same repository". I think this repo/package would be better off described in those terms, rather then how it achieves that function.

Ie, something like:

  • sphinx-monorepo
  • sphinx-readthedocs-monorepo
  • sphinx-multiproject
  • sphinx-readthedocs-multiproject

Copy link
Contributor

Choose a reason for hiding this comment

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

And so, for that:

Suggested change
Share a single `conf.py` file between several Sphinx projects.
useful if you want to import several projects from the same
repository on [Read the Docs](https://readthedocs.org).
Build multiple Sphinx projects from a single repository on [Read the Docs](https://readthedocs.org).

Copy link
Member Author

Choose a reason for hiding this comment

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

I like sphinx-readthedocs-multiproject :D

Copy link
Member Author

Choose a reason for hiding this comment

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

but it's also kind of long, so maybe sphinx-multiproject is okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

heh yeah, that is quite long. sphinx-multiproject seems like a good plan 👍

I could see us eventually maybe having a small collection of extensions, and maybe we have a sphinx-readthedocs at that point. This seems like it would be a good fit in that umbrella, as you're right that this is probably only really applicable to projects on RTD.

docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
tests/test_extension.py Outdated Show resolved Hide resolved
# Taken from
# https://github.com/sphinx-doc/sphinx/blob/586de7300a5269de93b4d813408ffdbf10ce2409/sphinx/config.py#L222-L222
pre_init_options = {"needs_sphinx", "suppress_warnings", "language", "locale_dirs"}
for option, value in docset_config.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

This method of overriding the configuration file seems redundant -- the user would already have independent values in docs/project1/conf.py and docs/project2/conf.py. Could we get away with doing an import of most of these values, instead of duplicating this data in a common config file?

I'm sure users will just want to do a:

import projecta.conf as projecta_conf
import projectb.conf as projectb_conf

sharedconf_docsets = {
    "projecta": {"config": {
        "project": proejcta.project,
        "version": projecta.version,
        ...
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea of having the settings in different files and also have the possibility of using just one file (if the user only needs to override a couple of values on each project).

I see this being used in different ways:

from multiproject.utils import get_project

extensions = [
    "sharedconf.extension",
    "sphinx.ext.intersphinx",
    "sphinx.ext.autodoc",
]

master_doc = "index"
language = "en"

multiproject_projects = {
    "api": {},
    "dev": {
        "config": {
            # Is this way of setting the options still useful?
            # Could be for simple projects,
            # so users don't need to import stuff.
            "project": "Dev project",
        },
    },
    "user": {},
    'auto': {
        # This is almost the same as using `config`,
        # but we set the values automatically from the `auto/conf.py` file?.
        # This has the same restrictions as using `config`.
        'config_file': 'auto/conf.py',
    },
}

current_project  = get_project(multiproject_projects)

# Set all values directly
# -----------------------

if current_project == 'api':
    from api.conf import *
elif current_project == 'user':
    from user.conf import *

# Set value by value
# ------------------

import api.conf as api_conf
import user.conf as user_conf
if current_project == 'api':
    config = api_conf
elif current_project == 'user':
    config =  user_conf

# Replace the original values
project = config.project
version = config.version
# Extending the original value
extensions += config.extensions

Copy link
Contributor

Choose a reason for hiding this comment

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

and also have the possibility of using just one file

Ahh I see where you're coming from. We could certainly support both. I do think the auto import method would be used the most, so I suppose I'm just advocating for adding that now/soon.

My guess would be that users would still maintain 2 full sphinx subprojects for more standard local development, so could see the multiproject_projects['dev'] being an advanced use case.

I did like how your original example had all the configuration that a project would need in one place, instead of requiring manual import from the user. Manual set up could be an advanced usage, but we would recommend something like your multiproject_projects['auto'] for almost all use cases.

setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@stsewd
Copy link
Member Author

stsewd commented Mar 10, 2022

I have updated the PR:

  • Rename extension to sphinx-multiproject
  • I have renamed the concept of "docset" to "project"
  • Read the values from {path}/conf.py by default (can be disabled)
  • I'm still keeping the "config" option on each project definition (but I'm fine removing it if you consider so)
  • Only test the latest release of sphinx 3.x, and all mayor releases of 4.x.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Looks great! I noted some copy change to add a little clarity to usage, but I mostly understood how to configure the extension either way.

Do we have a strong reason to use multiproject.extension as the extension import name? I believe we do this on our sphinx rtd extension, but it's not necessary and might make sense to match what other extensions do.

docs/configuration.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/configuration.rst Outdated Show resolved Hide resolved
docs/limitations.rst Outdated Show resolved Hide resolved
docs/quickstart.rst Outdated Show resolved Hide resolved
multiproject/__init__.py Outdated Show resolved Hide resolved
multiproject/extension.py Show resolved Hide resolved
@agjohnson
Copy link
Contributor

Changes look reasonable! I jumped in to get this merged today. We can revisit making version strings nice or automated, seems the solution still brought on cyclical imports.

@agjohnson agjohnson merged commit 5a1d963 into main Mar 18, 2022
@agjohnson agjohnson deleted the init branch March 18, 2022 04:07
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.

None yet

2 participants