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

SubConfig now catches failures and warnings #2710

Merged
merged 2 commits into from Mar 26, 2022

Conversation

smarie
Copy link
Contributor

@smarie smarie commented Dec 16, 2021

Fixes #2709

Copy link
Contributor

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

Sorry for the delay

@smarie
Copy link
Contributor Author

smarie commented Mar 25, 2022

Great, no worries @oprypin !

@oprypin oprypin merged commit d7fa905 into mkdocs:master Mar 26, 2022
@oprypin
Copy link
Contributor

oprypin commented Mar 26, 2022

Oops, unfortunately this makes a test fail.

https://github.com/mkdocs/mkdocs/runs/5702600210?check_suite_focus=true#step:5:19

FAIL: test_extra_context (mkdocs.tests.build_tests.BuildTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/mkdocs/mkdocs/mkdocs/tests/build_tests.py", line 181, in test_extra_context
    cfg = load_config(extra={'a': 1})
  File "/home/runner/work/mkdocs/mkdocs/mkdocs/tests/base.py", line 40, in load_config
    assert(errors_warnings == ([], [])), errors_warnings
AssertionError: ([], [('extra', ('a', 'Unrecognised configuration name: a'))])

That makes me think that there are probably unforeseen negative implications from this change, so I'll have to revert it until further investigation.

oprypin added a commit that referenced this pull request Mar 26, 2022
@smarie
Copy link
Contributor Author

smarie commented Mar 26, 2022

Indeed, sorry. Sad that the CI was not executed on PR :( . Maybe something to consider in the future ?

@smarie
Copy link
Contributor Author

smarie commented Mar 26, 2022

Update : I was able to test outside of tox, directly in PyCharm.
This is what happens :

  • the extra option in the default config schema is defined as an empty SubConfig() (without any options defined inside it)
    # extra is a mapping/dictionary of data that is passed to the template.
    # This allows template authors to require extra configuration that not
    # relevant to all themes and doesn't need to be explicitly supported by
    # MkDocs itself. A good example here would be including the current
    # project version.
    ('extra', config_options.SubConfig()),
  • in the failing test , the configuration is created with extra={'a': 1},
    def test_extra_context(self):
    cfg = load_config(extra={'a': 1})
    context = build.get_context(mock.Mock(), mock.Mock(), cfg)
    self.assertEqual(context['config']['extra']['a'], 1)

Therefore there was an error in the subconfig Unrecognised configuration name: a.
This error was until now, hidden : it was already there but was not caught nor shown to the user.

The fix is now doing its job correctly, showing the error to the user.

Now the decision to take with this test is "should the 'extra' config option in the default schema accept any kind of options inside it ?". If the answer is yes, we should probably define a constructor flag in SubConfig to either allow for unvalidated options or not. If the answer is no, in that case the test should be modified so that extra is passed an empty dict.

Let me know !

@oprypin
Copy link
Contributor

oprypin commented Mar 26, 2022

@smarie If I understand correctly, the extra option is now widely used as a freeform container of undefined options that themes can access.
Example: https://squidfunk.github.io/mkdocs-material/setup/setting-up-versioning/#versioning
Whatever change you develop, make sure this still works without warnings.

@smarie
Copy link
Contributor Author

smarie commented Mar 27, 2022

@oprypin I opened #2807 that fixes the failing test.

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 this pull request may close these issues.

SubConfig ignore configuration failures
2 participants