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

Fixes a bug when 'edit_url' is not defined in the config #1273

Merged
merged 1 commit into from Sep 8, 2017

Conversation

Projects
None yet
2 participants
@funkyfuture
Contributor

funkyfuture commented Sep 7, 2017

i encountered this tailed traceback:

  File "/home/user/mkdocs/mkdocs/nav.py", line 180, in __init__
    self._set_edit_url(config['repo_url'], config['edit_uri'])
  File "/home/user/mkdocs/mkdocs/nav.py", line 263, in _set_edit_url
    if not edit_uri.startswith('?') and not edit_uri.startswith('#'):
AttributeError: 'NoneType' object has no attribute 'startswith'
@waylan

This comment has been minimized.

Member

waylan commented Sep 7, 2017

Thanks for this. However, this needs a test that fails before the change and passes after before it will be accepted.

@funkyfuture funkyfuture force-pushed the funkyfuture:fix_no_edit_url branch from 4178c4f to 67b9882 Sep 7, 2017

@funkyfuture

This comment has been minimized.

Contributor

funkyfuture commented Sep 7, 2017

done. i realised the bug is only evoked when repo_name is not a default value.

@funkyfuture

This comment has been minimized.

Contributor

funkyfuture commented Sep 7, 2017

locally i rebased, so the fix is HEAD, but it appears different here.

@waylan

This comment has been minimized.

Member

waylan commented Sep 7, 2017

The tests for this are here. I was expecting something similar, not an integration test.

@funkyfuture

This comment has been minimized.

Contributor

funkyfuture commented Sep 7, 2017

well, i looked into this. but i couldn't evoke the bug with this test:

        repo_name = 'example'
        repo_url = 'http://example.com/'
        edit_uri = None

        site_navigation = nav.SiteNavigation({
            'pages': pages,
            'repo_name': repo_name,
            'repo_url': repo_url,
            'edit_uri': edit_uri,
            'docs_dir': 'docs',
            'site_dir': 'site',
            'site_url': '',
            'use_directory_urls': True
        })

am i missing something?

@waylan

This comment has been minimized.

Member

waylan commented Sep 7, 2017

Don't you want to test the case where repo_url = None (rather than edit_uri)?

@waylan

This comment has been minimized.

Member

waylan commented Sep 7, 2017

FYI, I just pushed a fix for the missing interpreters on Travis (see #1274 & #1275). If you rebase this, the tests should all run properly now.

@funkyfuture

This comment has been minimized.

Contributor

funkyfuture commented Sep 7, 2017

@waylan

This comment has been minimized.

Member

waylan commented Sep 7, 2017

Oh right. Sorry. Are you sure you rolled back your changes before trying your test? By my reading of the code, that should be correct.

Actually, I wonder if the appropriate fix would be to address this in config validation. Validation is already ensuring the edit_uri setting is a string (see mkdocs/config/defaults.py#L82) However, there is no default defined. Perhaps we should add default='' to avoid None as the default?

@funkyfuture

This comment has been minimized.

Contributor

funkyfuture commented Sep 7, 2017

@waylan

This comment has been minimized.

Member

waylan commented Sep 7, 2017

the edit_url could be expanded in the config as well.

You make a good point. While the edit_url is intentionally page specific (a separate URL for each page), combining the repo_url and edit_uri from the settings only needs to happen once. Perhaps that should happen in post-validation.

Truth be told, I haven't given much attention to this feature as I expect it will get ripped out and replaced with a (more flexible) plugin anyway (the plugin API is currently available in 1.0.dev). The recent changes in this space have all been from random one-time contributors. That said, we do have a bug, which should be fixed (by the way, this PR should probably be against the master branch).

@funkyfuture funkyfuture changed the base branch from 1.0.dev to master Sep 8, 2017

@funkyfuture funkyfuture force-pushed the funkyfuture:fix_no_edit_url branch from 67b9882 to 70562ce Sep 8, 2017

@funkyfuture funkyfuture force-pushed the funkyfuture:fix_no_edit_url branch from 70562ce to c7af2e5 Sep 8, 2017

@funkyfuture

This comment has been minimized.

Contributor

funkyfuture commented Sep 8, 2017

okay, i moved the logic to the config. i see that the feature has its flaws and really should be moved to a plugin. well, it works for now.

@waylan waylan merged commit 31de2ed into mkdocs:master Sep 8, 2017

3 checks passed

codecov/project 93.88% (target 90%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@waylan

This comment has been minimized.

Member

waylan commented Sep 8, 2017

@funkyfuture Thanks for the work on this. Greatly appreciated.

@funkyfuture funkyfuture deleted the funkyfuture:fix_no_edit_url branch Sep 9, 2017

@funkyfuture

This comment has been minimized.

Contributor

funkyfuture commented Sep 12, 2017

@waylan shall i prepare a pr that merges this into the 1.0.dev branch? i'd like to use this one with my use-case.

@waylan

This comment has been minimized.

Member

waylan commented Sep 12, 2017

I expect that part of the code will go through a major rewrite for 1.0, so I wasn't all that concerned about it.

@funkyfuture

This comment has been minimized.

Contributor

funkyfuture commented Sep 13, 2017

i could throw out that stuff, but w/o a substituting plugin. would you accept that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment