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

Use exclude keyword to skip markdown files #1370

Merged
merged 4 commits into from Jan 5, 2018

Conversation

Projects
None yet
2 participants
@brandongc
Contributor

brandongc commented Jan 4, 2018

This change slightly modifies the functionality of the copy_media_files() function by changing the behavior of the unused exclude option to be a list extensions rather than a list of Unix shell patterns.

This function is only called in mkdocs/commands/build.py and does not use the functionality.

I also add a variable to define markdown extensions in a single place.

This change will greatly simplify life for a plugin developer who wishes to reused this functionality (namely me).

So my plugin to copy markdown files can simply consist of:

utils.copy_media_files(config['docs_dir'], config['site_dir'], exclude = [], dirty=True)

in a post_build event.

I understand this may get rejected as there is a private refactor in progress for the next release, but please consider allowing for a similarly flexible util function in the future.

@waylan

This comment has been minimized.

Member

waylan commented Jan 4, 2018

First of all, exclude is not unused (see build.pyL276). It serves a very important function for themes that relies on Unix shell patterns (note that we are passing in more than file extensions). Apparently, we are not properly testing that as no tests are failing with your changes.

Second, why would you want to switch from the more powerful Unix shell patterns to file extensions only? That would be a regression to a less powerful solution.

That said, having Markdown extensions defined in one place makes sense. And using the exclude keyword to skip Markdown files seems like it could be a reasonable compromise. I might consider that change as long as we keep support for Unix shell patterns.

@brandongc

This comment has been minimized.

Contributor

brandongc commented Jan 4, 2018

Strange that I missed that call with grep - possibly due to it being multiline.

For the change away from patterns there is no motivation other than it appeared unused and was convenient for passing a list of extensions.

Some design choices:

  • Is it acceptable for exclude=['.ext'] to be interpreted as exclude=['*.ext']?

    If yes then I can add the logic for that to the copy_media_files().

    If no I can change markdown_extensions=[] to a list of shell patterns instead of extensions and change utils.is_markdown_file() accordingly.

  • Is having a default exclude=markdown_extensions ok?

@brandongc brandongc changed the title from refactor unused exclude option to Use exclude keyword to skip markdown files Jan 4, 2018

@brandongc

This comment has been minimized.

Contributor

brandongc commented Jan 4, 2018

The failed test is pip install codecov timing out

@brandongc brandongc force-pushed the brandongc:make-utils-friendly-for-plugins branch from ecc395b to a3e15d7 Jan 4, 2018

@waylan

waylan requested changes Jan 4, 2018 edited

Looks good. Just a few suggested changes and I would be happy to accept this.

'*.mkdn',
'*.mkd',
'*.md'
]

This comment has been minimized.

@waylan

waylan Jan 4, 2018

Member

Let's leave these as "extensions" (and call them that).

@@ -145,11 +153,12 @@ def clean_directory(directory):
os.unlink(path)
def copy_media_files(from_dir, to_dir, exclude=None, dirty=False):
def copy_media_files(from_dir, to_dir, exclude=markdown_patterns, dirty=False):

This comment has been minimized.

@waylan

waylan Jan 4, 2018

Member

Here you can use a list comprehension to convert the list of extensions to patterns:

 exclude=['*{0}'.format(x) for x in markdown_extensions]
'.mkd',
'.md',
]
return any(fnmatch.fnmatch(ext, pattern) for pattern in markdown_patterns)

This comment has been minimized.

@waylan

waylan Jan 4, 2018

Member

I like this, but I don't think we need to break out the extension from the path. Just pass path to fnmatch and remove line 232.

This comment has been minimized.

@waylan

waylan Jan 4, 2018

Member

Or, leave it unchanged as we have a list of extensions, not patterns. It probably should come down to which is more performant. If fnmatch is fast enough, then converting the extensions to patterns for each call might not matter. Otherwise, don't use fnmatch here.

@waylan

This comment has been minimized.

Member

waylan commented Jan 4, 2018

I restarted the timed-out job on Travis and it is good now.

@waylan

waylan approved these changes Jan 5, 2018

@waylan

Sorry I missed this before, but could you add an entry to the release notes? Just add a new section above the most recent release titled "Development Version" followed by a list item briefly summarizing this change and referencing this PR. Similar to what was done here. Thanks.

Otherwise, it looks good.

brandongc and others added some commits Jan 5, 2018

@waylan

waylan approved these changes Jan 5, 2018

@waylan waylan merged commit d00a8e5 into mkdocs:master Jan 5, 2018

3 checks passed

codecov/project 93.57% (target 90%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment