Simplify how the theme is override #67
Conversation
aladjadj
force-pushed
the
fix_theme_override
branch
from
c3c0348
to
44e94eb
Compare
|
It's necessary to add tests to validate that, I add them when I've some time |
aladjadj
force-pushed
the
fix_theme_override
branch
3 times, most recently
from
4894899
to
1234dab
Compare
There was a problem hiding this comment.
Hey @aladjadj! I noticed that mkdocs-techdocs-core==1.1.0 was breaking all documentation builds (due to this copy runtime error), so I went in and fixed just the runtime error, without any additional fixes (as you've done here).
My recommendation would be to rebase off of the latest main branch, and fix some of the underlying theme override logic in a new PR.
I've added notes below about what I discovered while testing out your branch; hope they help you get to the solution you're looking for!
aladjadj
force-pushed
the
fix_theme_override
branch
from
1234dab
to
e5a8cdc
Compare
|
Thx for your fix and comments and my apologize for the introduced bug. If the theme config is all the time an instance of |
aladjadj
changed the title
AttributeError: 'Theme' object has no attribute 'copy'
aladjadj
force-pushed
the
fix_theme_override
branch
5 times, most recently
from
106f891
to
9f9b034
Compare
aladjadj
force-pushed
the
fix_theme_override
branch
from
9f9b034
to
9dc5814
Compare
There was a problem hiding this comment.
No worries @aladjadj! I think the simplified approach you've got here is the way to go. I have one comment on logging, and then some test case ideas too.
Lemme know what you think!
Also \cc @camilaibs, who's on community rotation this week.
src/core.py
Outdated
| log.info("[mkdocs-techdocs-core] The theme has been replaced by 'material'") | ||
| log.info( | ||
| "[mkdocs-techdocs-core] Set theme.name to 'material' to override theme options" | ||
| ) |
There was a problem hiding this comment.
I'm a little worried that this will create undesirable noise in logs; my understanding is that most people would not have explicitly set a theme value, if they are using TechDocs, and thus, this message would start appearing on every build for most users.
Perhaps instead, it'd be good to log the opposite? E.g. when the theme name is detected to be material, it's likely that the user intended to override theme settings. In which case, a log message like Overridden theme settings in use (or something similar) would be helpful for them to see.
src/test_core.py
Outdated
| @@ -51,6 +51,17 @@ def test_override_default_config_with_user_config(self): | |||
| self.assertFalse(final_config["mdx_configs"]["toc"]["permalink"]) | |||
| self.assertTrue("mdx_truly_sane_lists" in final_config["markdown_extensions"]) | |||
|
|
|||
| def test_remove_theme_config_with_user_options(self): | |||
There was a problem hiding this comment.
The code path that gets run in this case is when config["theme"].name is not material, which results in theme configuration being overridden. ...But it's hard to tell that because the name of the theme is set somewhere else.
Maybe this test case would be clearer if you named it differently? Like test_theme_overrides_removed_when_name_is_not_material or something?
aladjadj
force-pushed
the
fix_theme_override
branch
from
de30102
to
57c4dfc
Compare
|
Thx for your help @iamEAP , i follow your different feedback to improve the PR. |
aladjadj
force-pushed
the
fix_theme_override
branch
from
57c4dfc
to
42ef6d5
Compare
Signed-off-by: Camila Belo <camilaibs@gmail.com>
This PR intends to simplify the code to update only the attribute necessary by backstage
Nota: Previously, this PR tried to fix the run-time error fixed by #69