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

Deprecation of Theme._vars #205

Closed
alexvoss opened this issue Oct 22, 2023 · 8 comments · Fixed by #212
Closed

Deprecation of Theme._vars #205

alexvoss opened this issue Oct 22, 2023 · 8 comments · Fixed by #212

Comments

@alexvoss
Copy link
Contributor

Hi all,

using the HEAD versions of mkdocs today, I stumbled across this:

mkdocs serve --watch-theme
INFO    -  Building documentation...
INFO    -  DeprecationWarning: Do not access Theme._vars, instead access the keys of Theme
           directly.
             File
           "/Users/avoss/src/mkdocs-material/devpt/venv/lib/python3.11/site-packages/mkdocs_rss_plugin/util.py",
           line 624, in guess_locale
               locale = mkdocs_config.get("theme")._vars.get("locale")
             File "/Users/avoss/src/mkdocs-material/devpt/mkdocs/mkdocs/theme.py", line
           81, in _vars
               warnings.warn(

This seems to have been introduced in MkDocs 1.5.0 (see bottom of the list of changes)? Not sure why I have not stumbled across this before...

Did a quick search and it seems there is one other place where this would need to be changed.

Hope this helps and that it is an obvious one-liner to fix.

Best wishes,

Alex

@alexvoss alexvoss changed the title Upcoming: deprecation of Theme._vars Deprecation of Theme._vars Oct 22, 2023
@Guts
Copy link
Owner

Guts commented Oct 23, 2023

Hi @alexvoss ,

Thanks for reporting. I try to fix it this week.
Or maybe you want to take the opportunity to make a PR? You've already did the 80% 😉

@alexvoss
Copy link
Contributor Author

I'll see if i can do a PR tomorrow, will put it on my todo list.

@alexvoss
Copy link
Contributor Author

Please bear with me. I want to see if I can get rid of the second usage of _vars as well but from a brief glance at the MkDocs code it is not entirely clear that a Theme as a language attribute. I also want to make sure both changes are covered by tests.

@alexvoss
Copy link
Contributor Author

Right, I think I have got somewhere. I did make changes to util.py to get rid of the usages of _var but on the way went down the rabbit-hole of trying to debug this by running the tests. I just want to document this here because it took me a long time to figure it out as I kept suspecting dead code and broken tests first:

To debug, you need to run pytest with --no-cov argument to enable debugging. This is not just a VSCode issue, it seems, so may be worth adding to the developer documentation?

I have rewritten the existing code to have what I think is equivalent functionality but slightly streamlined logic that helps with debugging. Got rid of the use of _vars and replaced it with the use of square brackets. Will fork, commit and do a PR now.

I do suspect that the test for language is dead code because the Theme class seems to always set a locale now? We can deal with this in a next step if I am not holding the wrong end of the stick on that.

@Guts
Copy link
Owner

Guts commented Oct 26, 2023

Please bear with me. I want to see if I can get rid of the second usage of _vars as well but from a brief glance at the MkDocs code it is not entirely clear that a Theme as a language attribute. I also want to make sure both changes are covered by tests.

The material theme has a language attribute: https://squidfunk.github.io/mkdocs-material/setup/changing-the-language/#site-language

@alexvoss
Copy link
Contributor Author

Right, I think I missed a comment in your code to the effect that these come from the theme.

@Guts
Copy link
Owner

Guts commented Dec 7, 2023

Released as part of https://github.com/Guts/mkdocs-rss-plugin/releases/tag/1.9.0

@alexvoss
Copy link
Contributor Author

alexvoss commented Dec 7, 2023

Thanks for this. I did a quick smoke test with the Material blog-rss integration example and it seems fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants