Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(post-asset): strip extensions better on permalink #5153

Merged
merged 2 commits into from
May 1, 2023

Conversation

syunaht
Copy link
Contributor

@syunaht syunaht commented Feb 2, 2023

What does it do?

Fixes #2134
Closes #2881

A better way to strip extensions on permalink in post-asset.js
To some degree, it is possible to increase plugin compatibility.

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

@github-actions
Copy link

github-actions bot commented Feb 2, 2023

How to test

git clone -b fix https://github.com/KagamigawaMeguri/hexo.git
cd hexo
npm install
npm test

Copy link
Member

@stevenjoezhang stevenjoezhang left a comment

Choose a reason for hiding this comment

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

Maybe you can consider modifying the hexo-abbrlink plugin instead?

@syunaht
Copy link
Contributor Author

syunaht commented Feb 2, 2023

Maybe you can consider modifying the hexo-abbrlink plugin instead?

Others have consulted the hexo-abbrlink plugin developer, and he thinks it's hexo's problem.
Details in ohroy/hexo-abbrlink#33

@yoshinorin
Copy link
Member

I'm not yet checking the behavior of post_asset.js.

But I think maybe we can merge this. Because this PR passes all of the current tests. Also, it includes #2881 for fixes #2134.

But we should change the PR title and source code comment. We should not change the source code for third-party plugins behavior.

@syunaht syunaht changed the title compatible with hexo-abbrlink fix(post-asset): strip extensions better on permalink Feb 7, 2023
@syunaht
Copy link
Contributor Author

syunaht commented Feb 7, 2023

I'm not yet checking the behavior of post_asset.js.

But I think maybe we can merge this. Because this PR passes all of the current tests. Also, it includes #2881 for fixes #2134.

But we should change the PR title and source code comment. We should not change the source code for third-party plugins behavior.

Thank you for your patience and guidance! Please review again if my changes match your explanation.

@yoshinorin yoshinorin mentioned this pull request Mar 9, 2023
6 tasks
Copy link
Member

@yoshinorin yoshinorin left a comment

Choose a reason for hiding this comment

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

@KagamigawaMeguri
Sorry for the late reply.
LGTM, I'll merge this. Thank you :)

@yoshinorin yoshinorin merged commit 8b95bbc into hexojs:master May 1, 2023
@yoshinorin yoshinorin added this to the 7.0.0 milestone May 1, 2023
dimaslanjaka added a commit to dimaslanjaka/hexo that referenced this pull request May 2, 2023
@D-Sketon
Copy link
Member

Is this a breaking change?
This PR doesn't pass the current tests.

@uiolee
Copy link
Member

uiolee commented Sep 24, 2023

It looks like this PR did not run check.

and I tested in my PC.

  1146 passing (16s)
  5 pending
  1 failing

  1) Hexo
       Models
         PostAsset
           path - virtual - when permalink contains .htm not in the end:

      AssertionError: expected '2023\09\24\bar\bar\foo.html' to deeply equal '2023\09\24\bar\.htm-foo\foo.html'
      + expected - actual

      -2023\09\24\bar\bar\foo.html
      +2023\09\24\bar\.htm-foo\foo.html

      at Context.<anonymous> (D:\git\hexo\test\scripts\models\post_asset.js:95:22)

@yoshinorin
Copy link
Member

yoshinorin commented Oct 7, 2023

@D-Sketon @uiolee
Sorry, I made some misunderstandings. Perhaps, I think we should revert this PR... 🙇‍♂️

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Won't generate with post_asset_folder on and permalink ending with ".html"
5 participants