-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Use exclude keyword to skip markdown files #1370
Conversation
First of all, 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 |
Strange that I missed that call with 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:
|
The failed test is |
ecc395b
to
a3e15d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a few suggested changes and I would be happy to accept this.
mkdocs/utils/__init__.py
Outdated
'*.mkdn', | ||
'*.mkd', | ||
'*.md' | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave these as "extensions" (and call them that).
mkdocs/utils/__init__.py
Outdated
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can use a list comprehension to convert the list of extensions to patterns:
exclude=['*{0}'.format(x) for x in markdown_extensions]
mkdocs/utils/__init__.py
Outdated
'.mkd', | ||
'.md', | ||
] | ||
return any(fnmatch.fnmatch(ext, pattern) for pattern in markdown_patterns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I restarted the timed-out job on Travis and it is good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
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.