-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[MRG] Patches sphinx's autosummary to handle case insensitive file systems #12968
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
[MRG] Patches sphinx's autosummary to handle case insensitive file systems #12968
Conversation
Hmmmm... Have you tried submitting a patch to sphinx-doc? I'd be more comfortable evaluating this change if I could see that patch directly at least. |
Fundamentally, the only addition to the try:
filename = app.config.custom_autosummary_file_map[name] + suffix
except KeyError:
filename = name + suffix It is a little crazy this is all that needs to be changed for this PR to work. scikit-learn must be doing something special to be able to link Anyways, I could try to patch sphinx, but that patch would most likely be on the latest version. Is there something preventing us from using the latest version of sphinx? |
I don't think there is anything preventing us using the latest version
|
I don't think it's so crazy that that is all that is needed. But it might
be nice to also consider how conflicts can be detected.
|
@jnothman I updated this PR with a smaller LOC fix to our issue. It mocks out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The monkey nature of this is messy, but I'm glad to have documentation working on windows...
return orig_os_path_join(a, name_with_suffix) | ||
|
||
os.path.join = custom_os_path_join | ||
process_generate_options(app) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use try-finally (or a context manager) when monkey patching
Is it possible to raise an error when there are conflicts, to make sure
this issue doesn't repeat?
|
doc/conf.py
Outdated
# a upper case module, i.e. `sklearn.cluster.DBSCAN` and | ||
# `sklearn.cluster.dbscan` | ||
custom_autosummary_file_map = { | ||
"sklearn.cluster.dbscan": "sklearn.cluster.dbscan_lowercase", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably be using some character that is invalid in identifiers as a delimiter, i.e. -
, not _
.
doc/conf.py
Outdated
custom_autosummary_file_map = { | ||
"sklearn.cluster.dbscan": "sklearn.cluster.dbscan_lowercase", | ||
"sklearn.cluster.optics": "sklearn.cluster.optics_lowercase" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sklearn.covariance.oas and sklearn.decomposition.fastica need the same treatment
I updated this PR with zero configuration approach to detecting if a file has already been generated with the same case insensitive name. In these cases the filename will be appended with the hash of the name. This can be seen here: https://43873-843222-gh.circle-artifacts.com/0/doc/modules/generated/sklearn.cluster.optic35a4574d84b18e6ad28c8c022895e64bb2242daf.html#sklearn.cluster.optics
|
Relying on "already processed" means processing order must be deterministic
and ideally ASCII ordering to preference the capitalised version
Sha1 is excessive when we really only care about uppercase or lowercase and
could produce a binary signature and base 64 encod it. But whatever
|
You have valid points. I’ll look into reducing the scope of this PR to prioritize the uppercase filename and add a predefined string to the lowercase filename. |
Here comes another update to this PR. The class names now take priority over the function names. When a function name matches a class name, then a prefix
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we assured that class names get processed before lowercase? Or does the algorithm not depend on that and I've misread it?
You are correct, this implementation still assumes that the class is generated first. It turns out this is always the case because sphinx sorts the names here. |
Overall I'm okay with this change as a temporary solution. I think it is
good if our docs compile on Windows, etc. but this is quite hacky. We need
a change like this to go in sphinx-doc that at least identifies the
conflict and allows the user to configure some resolution.
|
In creating this PR, I was trying to create a no configuration fix, so future PR's do not need to worry about this. Regarding a change to sphinx-doc, this problem stems from not being able to trust |
Why do we need os.path.isfile?
I think having a way to configure renames is a good start to a solution in
sphinx-doc, but keeping a history of generated paths in order to raise a
warning when duplicates area found also seems advisable
|
I suspect |
I have been wrestling with the following two approaches:
custom_autosummary_names_with_new_suffix = {
"sklearn.cluster.dbscan",
"sklearn.cluster.optics",
"sklearn.covariance.oas",
"sklearn.decomposition.fastica"
}
custom_autosummary_new_suffix = "-lowercase.rst" This leads to a simpler patch, but future contributors running into this issue would need to know to add their function to
@jnothman What do you think? |
As a reference, I opened another PR: #13022 that uses the configuration options. It feels less hacky because it does less and allow a user to configure the filenames. |
I am closing this PR in favor of #13022. |
Reference Issues/PRs
Fixes #12712
What does this implement/fix? Explain your changes.
Autogenerated functions that have the same case insensitive name as another generated file will include a hash of its name in its filename.
Any other comments?
This is a weird solution, since it manually replaces the
process_generate_options
.There will be a warning:
Since the new filename doesn't actually appear in any toctree.