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

feat: add permalink variable timestamp #5611

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

feat: add permalink variable timestamp #5611

wants to merge 2 commits into from

Conversation

uiolee
Copy link
Member

@uiolee uiolee commented Jan 9, 2025

What does it do?

fix #5586

Screenshots

Pull request tasks

Copy link

github-actions bot commented Jan 9, 2025

How to test

git clone -b timestamp https://github.com/hexojs/hexo.git
cd hexo
npm install
npm test

@uiolee uiolee force-pushed the timestamp branch 2 times, most recently from f819977 to a13326d Compare January 9, 2025 14:52
@uiolee uiolee marked this pull request as draft January 9, 2025 15:01
@uiolee

This comment was marked as outdated.

SukkaW
SukkaW previously approved these changes Jan 9, 2025
@coveralls
Copy link

coveralls commented Jan 14, 2025

Pull Request Test Coverage Report for Build 12764580920

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.482%

Totals Coverage Status
Change from base Build 12702421469: 0.0%
Covered Lines: 9411
Relevant Lines: 9460

💛 - Coveralls

@uiolee uiolee marked this pull request as ready for review January 14, 2025 05:49
Copy link
Member

@SukkaW SukkaW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add :update_timestamp as well IMHO?

@uiolee
Copy link
Member Author

uiolee commented Jan 14, 2025

Should we add :update_timestamp as well IMHO?

I'm not sure if there is such a use case. But generally speaking, permalink should not change often

Comment on lines 35 to 40
second: date.format('ss'),
i_month: date.format('M'),
i_day: date.format('D'),
timestamp: date.format('X'),
hash,
category: config.default_category
Copy link
Member

@SukkaW SukkaW Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought, we could use an approach like the following to only calculate the permalink value when needed, and cache it afterward:

function defineLazyProperty(object, propertyName, valueGetter) {
  const define = value => Object.defineProperty(object, propertyName, {value, enumerable: true, writable: true});
  Object.defineProperty(object, propertyName, {
    configurable: true, enumerable: true,
    get() {
      const result = valueGetter();
      define(result);
      return result;
    },
    set: define
  });
  return object;
}

const permalink = {};

defineLazyProperty(permalink, 'second', () => date.format('ss'));
// ...
defineLazyProperty(permalink, 'timestamp', () => date.format('X'));

Or we can use Map instead of object here.

We can do this in another PR.

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.

hexo permalink timestamp
3 participants