Skip to content

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

Merged
StrikerRUS merged 2 commits into
masterfrom
docs_autosummary
Aug 2, 2019
Merged

[docs] limit number of files to scan for autosummary generation and reorganize conf.py#2297
StrikerRUS merged 2 commits into
masterfrom
docs_autosummary

Conversation

@StrikerRUS
Copy link
Copy Markdown
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.

Comment thread docs/conf.py Outdated
autosummary_generate = ['Python-API.rst']

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

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
Copy Markdown
Collaborator Author

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
Copy Markdown
Contributor

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
Copy Markdown
Collaborator Author

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
@StrikerRUS StrikerRUS deleted the docs_autosummary branch August 2, 2019 11:51
@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.

3 participants