Fix custom_dir support and minification inconsistency#27
Fix custom_dir support and minification inconsistency#27wilhelmer merged 3 commits intobyrnereese:masterfrom
custom_dir support and minification inconsistency#27Conversation
|
No feedback and no merge, and I've seen the other PR merged 🤔Am I supposed to change something here @wilhelmer? |
|
To be honest, I don't know what to do with this. What's the issue with the |
|
This PR is safe to merge, it introduces no breaking changes for the end-user. The path in my - minify:
minify_html: !ENV [GMC_ENABLE_ON_PUBLISH, False]
minify_css: !ENV [GMC_ENABLE_ON_PUBLISH, False]
minify_js: !ENV [GMC_ENABLE_ON_PUBLISH, False]
cache_safe: !ENV [GMC_ENABLE_ON_PUBLISH, False]
htmlmin_opts:
remove_comments: true
css_files:
- assets/stylesheets/extra.css
js_files:
- assets/javascripts/extra.jsThe difference is that I moved all of my shared assets into the mkdocs-minify-plugin/mkdocs_minify_plugin/plugin.py Lines 148 to 150 in cecdab8 because it expected that the file will be located within the docs_dir, but right now it's not the case, so I fixed it by finding the first valid file path in a custom_dir of the theme aka the overrides directory in my case.
The second issue was that the new mkdocs-minify-plugin/mkdocs_minify_plugin/plugin.py Lines 147 to 157 in e0ab4d9 and you probably want to handle the js_min the same way even if the cache_safe setting is disabled 😄.The error was hidden because the current tests didn't properly check JavaScript minification with different settings enabled. |
| if self.config["cache_safe"]: | ||
| docs_file_path: str = f"{config['docs_dir']}/{file_path}".replace("\\", "/") | ||
|
|
||
| for user_config in config.user_configs: |
There was a problem hiding this comment.
It caught me off guard that someone is actually using user_configs in MkDocs. I thought it's there in MkDocs only by accident. Could you explain why it was used here, I'm curious. I thought that one could just read config.theme directly and get the same outcome.
There was a problem hiding this comment.
I used it since otherwise I would have to use config.theme._vars, and since it's a pythonic-private variable I preferred to use a public one.
I did the same in the social cards plugin in MkDocs Material.
Hello again,
my team started supporting multiple languages in the docs, and I moved the shared extra assets to our overrides directory to avoid file duplication. This created an issue with the
cache_safeoption. Seems like people don't use this setting, because otherwise this issue would be found sooner. Maybe people prefer less "invasive" ways like this one, however I'm not sure if that's all there is to it.Additionally, I've unified the minification when using and not using the
cache_safeoption.The order of functions makes the code still hard to read and maybe has caused the bug in the first place
My attempt at restructuring wasn't accepted, because I did too many things back then #19 and then opted into a less invasive revision, but I still think it's worth a shot.
You could add some "easy-first-commit" issues to: