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

Optional items in FILENAME_METADATA expression. #2117

Merged
merged 1 commit into from Mar 20, 2017

Conversation

Projects
None yet
4 participants
@timwienk
Contributor

timwienk commented Mar 11, 2017

To keep my articles somewhat organised, I like to add the date to the filename of the article and extract it using FILENAME_METADATA. However, for pages I don't quite want to do that. I used to use a simple plugin to set the date based on mtime, but since DEFAULT_DATE = 'fs' uses mtime now (since December 2016) instead of ctime, this option is great for this.

However, things go wrong when making the date capture group optional in the FILENAME_METADATA expression. Previously I monkey patched the pelican.readers.parse_path_metadata function, but since the DEFAULT_DATE = 'fs' option changed, I figured it's a good idea to try and get this part of the date configuration sorted within Pelican itself as well.

Case:

files:

  • content/index.md
  • content/articles/2017-03-11.interesting-article.md

pelicanconf.py:

FILENAME_METADATA = '(?:(?P<date>\d{4}-\d{2}-\d{2})\.)?(?P<slug>.*)\.md'

Error:

The regex match result for the "date" group is None for content/index.md. When Pelican attempts to process the date, None.replace() throws an AttributeError: 'NoneType' object has no attribute 'replace'.

Solution:

In addition to checking if k not in metadata when applying the matched filename metadata, also check if v is not None.

Please see the commit in the pull request for the actual change + added tests.

@ingwinlu

lgtm

@@ -672,7 +672,7 @@ def parse_path_metadata(source_path, settings=None, process=None):
# .items() for py3k compat.
for k, v in match.groupdict().items():
k = k.lower() # metadata must be lowercase
if k not in metadata:
if k not in metadata and v is not None:

This comment has been minimized.

@avaris

avaris Mar 15, 2017

Member

Not that big of a deal but I'd prefer switching the comparison order for performance reasons. Comparison to None would be faster and should short-circuit earlier:

if v is not None and k not in metadata:

This comment has been minimized.

@timwienk

timwienk Mar 15, 2017

Contributor

I agree, that makes sense. I'll swap the order and replace this commit.

Fix setting None metadata from FILENAME_METADATA matches.
This is relevant when using optional items in the expression. E.g. if an
optional captured group is not matched, the result of
`match.groupdict()` contains the captured group with value `None`.

@timwienk timwienk force-pushed the timwienk:fix-none-filename-metadata branch from 97dafde to 4917b86 Mar 15, 2017

@avaris

avaris approved these changes Mar 15, 2017

👍

@timwienk timwienk changed the title from Optional items in FILE_METADATA expression. to Optional items in FILENAME_METADATA expression. Mar 17, 2017

@justinmayer

This comment has been minimized.

Member

justinmayer commented Mar 20, 2017

Many thanks to @avaris and @ingwinlu for reviewing this pull request.

@justinmayer justinmayer added this to the 3.8.0 milestone Mar 20, 2017

@justinmayer

This comment has been minimized.

Member

justinmayer commented Mar 20, 2017

You are my new hero, Tim. Why, you ask?

  • clear description of your intended use case, perceived problem, and proposed solution
  • rebased & squashed into a single commit — without having to be asked
  • a single line change accompanied by three dozen lines of tests (!)

Thank you for fulfilling every project maintainer's dream. 😃 👍

@justinmayer justinmayer merged commit fb07f4d into getpelican:master Mar 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@timwienk timwienk deleted the timwienk:fix-none-filename-metadata branch Mar 21, 2017

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