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

Add autosummary_filename_map config to avoid clashes #7927

Merged
merged 8 commits into from Jul 23, 2020

Conversation

jnothman
Copy link
Contributor

@jnothman jnothman commented Jul 8, 2020

Subject: Add autosummary_filename_map config to allow users to configure alternative filenames for autodoc objects, avoiding name clashes on case-insensitive file systems.

Feature or Bugfix

  • Feature
  • Bugfix

Purpose

  • This allows the user to configure the mapping between object name and filename when performing autodoc generation of HTML files. It adds a config param called autosummary_filename_map to do so. It is a simple dict between name and filename to be generated.
  • The intention is that this will allow renames on case-insensitive file systems (per Autodoc may overwrite where functions and classes with different case exist #2024), albeit requiring users to manually identify filenames that might result in name clashes, and propose an alternative filename for the generated docs.

Relates

@jnothman
Copy link
Contributor Author

jnothman commented Jul 8, 2020

[fixed branching and force-pushed.]

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits! I'll merge this after your update soon :-)

tests/roots/test-autosummary/sphinx.rst Outdated Show resolved Hide resolved
tests/roots/test-ext-autosummary-filename-map/index.rst Outdated Show resolved Hide resolved
tests/test_ext_autosummary.py Outdated Show resolved Hide resolved
tests/roots/test-ext-autosummary-filename-map/conf.py Outdated Show resolved Hide resolved
sphinx/ext/autosummary/generate.py Show resolved Hide resolved
@tk0miya tk0miya added this to the 3.2.0 milestone Jul 9, 2020
@jnothman
Copy link
Contributor Author

Thank you for the review. I hope I have addressed all comments.

filename_map = app.config.autosummary_filename_map
if not isinstance(filename_map, Mapping):
raise TypeError('autosummary_filename_map should be a mapping from '
'strings to strings')
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to check the value is dict here. The Sphinx automatically checks the given value is the same type as the default value. But it does not check the contents of the dict. I think the default check is enough.

Note: If you'd like to add more careful validations, please use a hook for the config-inited event to validate user configurations.

def validate_latex_theme_options(app: Sphinx, config: Config) -> None:
for key in list(config.latex_theme_options):
if key not in Theme.UPDATABLE_KEYS:
msg = __("Unknown theme option: latex_theme_options[%r], ignored.")
logger.warning(msg % (key,))
config.latex_theme_options.pop(key)

app.connect('config-inited', validate_latex_theme_options, priority=800)

@@ -790,5 +790,6 @@ def setup(app: Sphinx) -> Dict[str, Any]:
app.add_config_value('autosummary_mock_imports',
lambda config: config.autodoc_mock_imports, 'env')
app.add_config_value('autosummary_imported_members', [], False, [bool])
app.add_config_value('autosummary_filename_map', {}, 'html')
Copy link
Member

Choose a reason for hiding this comment

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

It is better to add a new config variable alphabetically. Could you move this above?
(I noticed autosummary_imported_members is not. I'll fix it later)

@thomasjpfan
Copy link

thomasjpfan commented Jul 13, 2020

Tried this for scikit-learn and got an expected warning:

checking consistency... /Users/thomasfan/Repos/scikit-learn-1/doc/modules/generated/dbscan-lowercase.rst: 
WARNING: document isn't included in any toctree
done

@tk0miya
Copy link
Member

tk0miya commented Jul 13, 2020

Oops. Autosummary.run() is also fixed using the new configuration. It constructs a toctree node as a reference to the document file.

@jnothman
Copy link
Contributor Author

jnothman commented Jul 13, 2020 via email

@tk0miya
Copy link
Member

tk0miya commented Jul 14, 2020

I think some fix is needed to here. Is my understanding incorrect?

for name, sig, summary, real_name in items:
docname = posixpath.join(tree_prefix, real_name)
docname = posixpath.normpath(posixpath.join(dirname, docname))

@jnothman
Copy link
Contributor Author

I think some fix is needed to here. Is my understanding incorrect?

Seems reasonable. I suppose I need to come up with a test case.

@jnothman
Copy link
Contributor Author

This is ready for review.

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits. I'll merge this after the fix soon.

tests/roots/test-ext-autosummary-filename-map/index.rst Outdated Show resolved Hide resolved
@tk0miya tk0miya merged commit da17413 into sphinx-doc:3.x Jul 23, 2020
@tk0miya
Copy link
Member

tk0miya commented Jul 23, 2020

Just merged. Thank you for your contribution!

tk0miya added a commit that referenced this pull request Jul 23, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autodoc may overwrite where functions and classes with different case exist
3 participants