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

[docs] limit number of files to scan for autosummary generation and reorganize conf.py #2297

Merged
merged 2 commits into from Aug 2, 2019

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Jul 31, 2019

Refer to #2286 (comment).

Before:

D:\Users\nekit\Downloads\LightGBM\docs>make html
Running Sphinx v1.8.5
making output directory...
[autosummary] generating autosummary for: Advanced-Topics.rst, C-API.rst, Development-Guide.rst, Experiments.rst, FAQ.rst, Features.rst, GPU-Performance.rst, GPU-Targets.rst, GPU-Tutorial.rst, GPU-Windows.rst, Installation-Guide.rst, Parallel-Learning-Guide.rst, Parameters-Tuning.rst, Parameters.rst, Python-API.rst, Python-Intro.rst, Quick-Start.rst, README.rst, gcc-Tips.rst, index.rst

...

Now:

D:\Users\nekit\Downloads\LightGBM\docs>make html
Running Sphinx v1.8.5
making output directory...
[autosummary] generating autosummary for: Python-API.rst

...

@hayesall Can you please give your review?

UPD: Also, group options in conf.py logically.

docs/conf.py Outdated
@@ -81,12 +81,12 @@ def run(self):
"show-inheritance": True,
}

# Generate autosummary pages. Output should be set with: `:toctree: pythonapi/`
autosummary_generate = ['Python-API.rst']

# Add any paths that contain templates here, relative to this directory.
templates_path = ['_templates']
Copy link
Contributor

@hayesall hayesall Jul 31, 2019

Choose a reason for hiding this comment

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

The only change I would suggest is moving these lines back below the templates_path variable to keep the diffs smaller.

Copy link
Collaborator Author

@StrikerRUS StrikerRUS Jul 31, 2019

Choose a reason for hiding this comment

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

@hayesall Thanks for your review! I moved autosummary_generate variable with the aim to keep logic grouping: autodoc_* variables are closer to autosummary_* and templates_path is closer to source_suffix, so the first group is about auto_ extensions and the second one is about source file paths.

IDK, maybe you're right and smaller diff is more important than grouping. Let's wait for one more opinion from another reviewer...

Copy link
Contributor

@hayesall hayesall Jul 31, 2019

Choose a reason for hiding this comment

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

@StrikerRUS Logical ordering is more important small diffs, I like this now that I see it this way 👍

While we're on the subject: it doesn't appear that templates are used in this repository, so we could drop the templates_path variable.

Copy link
Collaborator Author

@StrikerRUS StrikerRUS Jul 31, 2019

Choose a reason for hiding this comment

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

@hayesall Good suggestion! Thanks! I also dropped commented option and reorganized two other options connected to extensions in the latest commit 98ebbd8.

@StrikerRUS StrikerRUS changed the title [docs] limit number of files to scan for autosummary generation [docs] limit number of files to scan for autosummary generation and reorganize conf.py Jul 31, 2019
@StrikerRUS StrikerRUS merged commit f68e1b5 into master Aug 2, 2019
6 checks passed
@StrikerRUS StrikerRUS deleted the docs_autosummary branch Aug 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants