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

Added null-coalescing check for published date #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added null-coalescing check for published date #2

wants to merge 3 commits into from

Conversation

dg01d
Copy link

@dg01d dg01d commented Jun 8, 2018

This small adjustment checks that there is a published date set on the incoming webmention.

If published is null, then it is set to the same datestamp as wm-received.

This deals with errors created in Hugo where "published": null causes a failure to build.

@martymcguire
Copy link
Owner

Thanks for this PR!

Sent a PR to match up the whitespace w/ the rest of the file (unexpanded tabs, soorrry).

I also wanted to chat a bit about this change because I thought about adding this the first time through but decided to leave it out.

My thinking at the time was that if the original is missing a 'published' property, that might be intentional. For example, a reference page or something else non-date-y. It felt somewhat misleading to use the wm-received date in place of a published date, since it could be any time, as a mention doesn't have to be sent by the original publisher.

In my current templates, I check whether published is set before using it, and just omit it if absent.

I may be worrying too much, so I'd love to hear your thoughts!

@dg01d
Copy link
Author

dg01d commented Jun 8, 2018

Merged your AWFUL whitespace.

@dg01d
Copy link
Author

dg01d commented Jun 9, 2018

Hey Marty,

I didn't see your comment until this morning.

I can understand why you chose this path; it does make sense that the published date is available only where that data has relevance. I guess my templating skills aren't as good as yours.

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.

2 participants