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

Fix attempt: MySubConfig has no '_pre_validate' #59

Closed
wants to merge 3 commits into from

Conversation

GenevieveBuckley
Copy link
Contributor

(maybe) closes #57

The way MySubConfig is currently implemented is incompatible with mkdocs versions above 1.2.4. I think getting rid of it entirely removes the issue?

  • It worked when I made this change and then build the docs with the python version nox made:
    .nox/tests-3.9/bin/python ../mkdocs/mkdocs/__main__.py build
    .nox/tests-3.9/bin/python ../mkdocs/mkdocs/__main__.py serve
    
  • I still had some problems trying to run mkdocs build directly with my own virtual environment. There's probably something wrong with my environment, so it'd be very helpful if you could try this too on your own machine.

It's also not entirely clear to me why the MySubConfig class exists. It looks like there are two reasons:

  1. So you can return an empty dict instead of default values if no info is provided by the user
    • The docstring says "Same as SubConfig except that it will be an empty dict when nothing is provided by user, instead of a dict with all options containing their default values." see this commit (it's a big commit, you have to scroll down to see the relevant part in plugin.py).
    • But I don't know why that's important, or what could go wrong if we take it out. Maybe you can weigh in @smarie?
  2. There is also a fix compensating for a bug in mkdocs see this commit, which you later fixed upstream here. I think that means we don't need that part here now?

@smarie
Copy link
Owner

smarie commented Apr 1, 2023

Thanks @GenevieveBuckley ! Sorry for the delay with my responses. I triggered the CI. I'll have a look at both your PRs in the upcoming week hopefully

@GenevieveBuckley
Copy link
Contributor Author

Hmm, test_minimal_conf is failing:

Expand for details:
=================================== FAILURES ===================================
 ______________________________ test_minimal_conf _______________________________
 
 basic_mkdocs_config = {'config_file_path': '/tmp/pytest-of-runner/pytest-0/test_minimal_conf0/mkdocs.yml', 'site_name': 'basic_conf', 'nav':...a': {}, 'plugins': {'search': <mkdocs.contrib.search.SearchPlugin object at 0x7f2af3a559a0>}, 'hooks': {}, 'watch': []}
 
     def test_minimal_conf(basic_mkdocs_config):
         """Test that minimal config can be loaded without problem"""
 
         # Create a mini config
         mini_config = yaml_load("""
     examples_dirs: docs/examples          # path to your example scripts
     gallery_dirs: docs/generated/gallery  # where to save generated gallery
     """)
 
         # Load it (this triggers validation and default values according to the plugin config schema)
         plugin = GalleryPlugin()
         plugin.load_config(mini_config)
 
         # Now mimic the on_config event
 >       result = plugin.on_config(basic_mkdocs_config)
 
 /home/runner/work/mkdocs-gallery/mkdocs-gallery/tests/test_config.py:47:
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 /home/runner/work/mkdocs-gallery/mkdocs-gallery/.nox/tests-3-9/lib/python3.9/site-packages/mkdocs_gallery/plugin.py:189: in on_config
     self.config = parse_config(self.config, mkdocs_conf=config)
 /home/runner/work/mkdocs-gallery/mkdocs-gallery/.nox/tests-3-9/lib/python3.9/site-packages/mkdocs_gallery/gen_gallery.py:134: in parse_config
     gallery_conf = _complete_gallery_conf(gallery_conf, mkdocs_conf=mkdocs_conf, check_keys=check_keys)
 /home/runner/work/mkdocs-gallery/mkdocs-gallery/.nox/tests-3-9/lib/python3.9/site-packages/mkdocs_gallery/gen_gallery.py:381: in _complete_gallery_conf
     gallery_conf['binder'] = check_binder_conf(gallery_conf['binder'])
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
 
 binder_conf = {'binderhub_url': 'https://mybinder.org/', 'branch': 'gh-pages', 'dependencies': None, 'filepath_prefix': None, ...}
 
     def check_binder_conf(binder_conf):
         """Check to make sure that the Binder configuration is correct."""
 
         # Grab the configuration and return None if it's not configured
         binder_conf = {} if binder_conf is None else binder_conf
         if not isinstance(binder_conf, dict):
             raise ConfigError('`binder_conf` must be a dictionary or None.')
         if len(binder_conf) == 0:
             return binder_conf
 
         # Ensure all fields are populated
         req_values = ['binderhub_url', 'org', 'repo', 'branch', 'dependencies']
         optional_values = ['filepath_prefix', 'notebooks_dir', 'use_jupyter_lab']
         missing_values = []
         for val in req_values:
             if binder_conf.get(val) is None:
                 missing_values.append(val)
 
         if len(missing_values) > 0:
 >           raise ConfigError(f"binder_conf is missing values for: {missing_values}")
 E           mkdocs_gallery.errors.ConfigError: binder_conf is missing values for: ['org', 'repo', 'dependencies']

</details>

@GenevieveBuckley
Copy link
Contributor Author

GenevieveBuckley commented Apr 3, 2023

When I try to run the nox tests, I get an unhashable type: 'list' error. But the same if/then/else check works well for me in an interactive ipython session, and I can't see which object still thinks its a python list. EDIT: nevermind, figured it out!

@GenevieveBuckley
Copy link
Contributor Author

The nox tests pass for me now, I think this is a decent fix.

default_values = [None, 'gh-pages', 'https://mybinder.org']
if vals == []:
binder_conf = {}
elif all([val in default_values for val in vals]):
Copy link
Owner

@smarie smarie Apr 11, 2023

Choose a reason for hiding this comment

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

This is not very satisfying. What if someone sets 'branch' to 'https://mybinder.org' ? Also it duplicates the information in GalleryPlugin . I would rather find a better way to do this with mkdocs: If you can find your way through mkdocs config class to create a "fixed" MySubConfig it would probably be better.

I'll try to have a look one of these days ; but as you could notice I have a hard time finding a few minutes to code :( sorry @GenevieveBuckley

@smarie
Copy link
Owner

smarie commented Apr 12, 2023

I think that I found a solution that is more in the spirit of the new mkdocs configuration mechanism: #60

Therefore we can probably close this PR now. Thanks a lot @GenevieveBuckley for pointing me in the right direction !

@smarie smarie closed this Apr 12, 2023
@GenevieveBuckley
Copy link
Contributor Author

I think that I found a solution that is more in the spirit of the new mkdocs configuration mechanism: #60

Therefore we can probably close this PR now. Thanks a lot @GenevieveBuckley for pointing me in the right direction !

Wonderful! Thank you so much for finding a better fix for this 😄

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.

AttributeError: 'MySubConfig' object has no attribute '_pre_validate'
2 participants