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

Use date instead of file mtime for updated date #3235

Merged
merged 8 commits into from Sep 21, 2019

Conversation

@tomap
Copy link
Contributor

tomap commented Aug 18, 2018

Because this date is unreliable when cloning a repository from git
matches the last clone instead of lat update
This is particularly important when used in cunjunction with hexo-generator-feed

Thank you for creating a pull request to contribute to Hexo code! Before you open the request please review the following guidelines and tips to help it be more easily integrated:

  • Add test cases for the changes.
  • Passed the CI test.
Because this date is unreliable when cloning a repository from git
matches the last clone instead of lat update
This is particularly important when used in cunjunction with hexo-generator-feed
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 18, 2018

Coverage Status

Coverage increased (+0.01%) to 97.157% when pulling 677f5b8 on tomap:master into 00151a1 on hexojs:master.

@tcrowe

This comment has been minimized.

Copy link
Contributor

tcrowe commented Sep 29, 2018

Hi @tomap. It seems like a good idea. Where does the updated variable show up in the site?

@tomap

This comment has been minimized.

Copy link
Contributor Author

tomap commented Oct 5, 2018

My use case is in feed plugin where the update date of a post makes no sense in my case, where I would want them to not use the file dates, but instead use the date, when no update is set.

https://github.com/hexojs/hexo-generator-feed/blob/master/atom.xml#L24

However, after giving it some thought, I think it is more an option of feed to introduce or a new post property, otherwise, it will create a change of behavior

@tcrowe

This comment has been minimized.

Copy link
Contributor

tcrowe commented Oct 5, 2018

@tomap Should we ask hexo core team their thoughts and see if anyone wants to talk more about this? I cannot yet understand what depends on these dates but it seems like a good idea you had.

@tomap

This comment has been minimized.

Copy link
Contributor Author

tomap commented Oct 6, 2018

Yes, we should, especially given the breaking change possibility

@NoahDragon

This comment has been minimized.

Copy link
Member

NoahDragon commented Nov 2, 2018

IMHO, this will change the purpose of the updated date field. I know some plugins use this field to set the updated date for the posts. Why not we modify the hexo-generator-feed to handle the date correctly, instead of changing the hexo itself?

@tomap

This comment has been minimized.

Copy link
Contributor Author

tomap commented Nov 3, 2018

Agreed. I'll do a PR there

@tomap tomap closed this Nov 3, 2018
@tcrowe tcrowe mentioned this pull request Dec 3, 2018
@tomap

This comment has been minimized.

Copy link
Contributor Author

tomap commented Jul 10, 2019

Hi, I thougth a bit more about it and it is important to have this change inside hexo.
Here is why:
1/ if I do it at the plugin level, I'll have two variables available:

  • Date (date of publication)
  • Updated, which will be either the "true" updated date (from the front-matter), and a meaningless date (mtime) but with no way for me to kwno which is which

2/ If I do it at hexo level (with an option, for backward compatibility)

  • Date (date of publication)
  • Updated, which will be either the "true" updated date (from the front-matter), or the date of publication (if no updated date is available)

So I'm reopening this PR, and will amend it soon to include the new option for backward compatility (backward compatible behavior if false or unset)

@tomap tomap reopened this Jul 10, 2019
@SukkaW

This comment has been minimized.

Copy link
Member

SukkaW commented Jul 11, 2019

Using file mtime as updated affect not only hexo-generator-feed but also any theme using variable post/page.updated.

For example, my hexo-theme-suka supports a feature which based on post/page.updated:

image

In my case, I believe it is a b better option to use date as updated when no updated is given in front-matter.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Jul 25, 2019

I prefer not dropping mtime, it's quite useful, although I personally prefer to manually set the post.updated. I'm ok with introducing new option (as shown in your latest commit.

Would updated: (in similar format as date:) in front matter resolve the issue you mentioned? Kinda similar approach as #3612.

@tomap

This comment has been minimized.

Copy link
Contributor Author

tomap commented Jul 30, 2019

@curbengh indeed, mentionning updated in my posts would solve the situation, but I don't want to have to mention them to fix the issue.
I want the default to work in my case
That is why I'm introducing a new option

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Jul 30, 2019

These two seem to be conflicting:

Updated, which will be either the "true" updated date (from the front-matter), or the date of publication (if no updated date is available)
// Use post date for updated date instead of file modification date when front-matter provides no date

shouldn't it be "...when front-matter provides no updated date"?

@tomap

This comment has been minimized.

Copy link
Contributor Author

tomap commented Jul 30, 2019

These two seem to be conflicting:

Updated, which will be either the "true" updated date (from the front-matter), or the date of publication (if no updated date is available)
// Use post date for updated date instead of file modification date when front-matter provides no date

shouldn't it be "...when front-matter provides no updated date"?

I fixed that. Thank you

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Jul 31, 2019

Thanks. I'll test it in the meantime.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Aug 6, 2019

"lib/plugins/processor/asset.js" only affects page, similar lines also need to be added to "post.js".

tomap added 2 commits Aug 19, 2019
Copy link
Contributor

curbengh left a comment

Works fine in my local test.

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Aug 19, 2019

Note to theme dev, If a theme currently display updated date, it needs to add another condition:

if (post.updated && post.updated.toString() !== post.date.toString()) { %>

Edit: my comment below has better option.

@tomap

This comment has been minimized.

Copy link
Contributor Author

tomap commented Aug 20, 2019

Or we could introduce an easier property that would leave the updated property empty in case it is not set in front matter?

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Aug 20, 2019

The condition I mentioned is for compatibility when use_date_for_updated is enabled. Now I think about it, the following condition works better,

<% if (post.updated && !config.use_date_for_updated) { %>

Edit: Just realized the above condition will not display post.updated if use_date_for_updated is enabled. My previous comment still works.

@SukkaW SukkaW added this to the v4.0.0 milestone Aug 25, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Aug 27, 2019

Or we could introduce an easier property that would leave the updated property empty in case it is not set in front matter?

I would prefer that option, so if (post.updated) works better.

mtime_updated perhaps?

@curbengh

This comment has been minimized.

Copy link
Contributor

curbengh commented Sep 18, 2019

@tomap

Or we could introduce an easier property that would leave the updated property empty in case it is not set in front matter?

Are you going to implement that option in this PR or later? Otherwise, this is good to go.

@tomap

This comment has been minimized.

Copy link
Contributor Author

tomap commented Sep 20, 2019

I'm not going to change this PR.

@curbengh curbengh merged commit 6f6d04e into hexojs:master Sep 21, 2019
3 of 4 checks passed
3 of 4 checks passed
codeclimate 2 issues to fix
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 97.157%
Details
curbengh added a commit to curbengh/hexo-starter that referenced this pull request Oct 11, 2019
- pretty_urls.trailing_index: hexojs/hexo#3691
- external_link.enable: hexojs/hexo#3675
- meta_generator: hexojs/hexo#3653
- use_date_for_updated: hexojs/hexo#3235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.