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

code block in markdown are not rendered correctly after set disableNunjucks to true #3573

Merged
merged 3 commits into from Jun 16, 2019

Conversation

6 participants
@think-in-universe
Copy link
Contributor

commented Jun 4, 2019

Fix the issue that code blocks in markdown are rendered as placeholder after disabling nunjucks

What does it do?

Fix the issue that "code block in markdown are not rendered correctly after set disableNunjucks to true", which is introduced in PR #2593

Expected behavior:

  1. most tag plugins (nunjucks) feature should be disabled after set disableNunjucks to true ;
  2. however, markdown features like code blocks should still work;

How to test

(1) Hexo Test

git clone -b master https://github.com/think-in-universe/hexo.git
cd hexo
npm install
npm test     # passed in my local test

(2) Test "Disable Nunjucks" At Renderer-Level

To test disableNunjucks, we can use the hexo-stop-tag-plugins plugin created by me. The plugin has been submitted to the plugins list via the PR hexojs/site#966

git clone -b master https://github.com/think-in-universe/hexo-stop-tag-plugins.git
cd hexo-stop-tag-plugins
npm install
npm test    # 4 of 11 test failed
npm install think-in-universe/hexo#master   # override the hexo package
npm test   # 11 or 11 tests passed, after include the change in this PR

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.
@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

For "Add test cases for the changes.", what the recommended ways to add that?

@coveralls

This comment has been minimized.

Copy link

commented Jun 4, 2019

Coverage Status

Coverage remained the same at 97.265% when pulling 995097e on think-in-universe:master into 1b432c3 on hexojs:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Jun 4, 2019

Coverage Status

Coverage remained the same at 97.265% when pulling 995097e on think-in-universe:master into 1b432c3 on hexojs:master.

@coveralls

This comment has been minimized.

Copy link

commented Jun 4, 2019

Coverage Status

Coverage decreased (-0.1%) to 97.148% when pulling 6e6ee20 on think-in-universe:master into 1b432c3 on hexojs:master.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2019

hello, when will anyone look at this PR? @YoshinoriN @weyusi

@tomap

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2019

Hi, why go to the trouble of disabling nunjucks, instead of using the raw tag?

https://mozilla.github.io/nunjucks/templating.html#raw

I haven't tested it, but that could help, isn't it?

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@tomap I think there're already numerous discussion about disabling nunjucks, such as:

  1. #2384
  2. #1510
  3. #1755
  4. #2593

Using {% raw %} doesn't work for quite some use case, when you just want to use pure markdown syntax, without nunjucks.

For my use case, I cannot change the markdown source code written by my friends (which are conflicted with nunjucks somehow), but I still want to help them to convert markdown into HTML pages, because I cannot force the writers to use nunjucks by modifying their source intensively.

From another perspective, the disableNunjucks property was not introduced by myself, and it's from the PR #2593. But it doesn't work well according to my test, so I try to fix it.

For the design of hexo, I don't think "disabling nunjucks" is kind of trouble. On the contrary, it can provide more flexibility for hexo users/developers who want to use customized renderers.

For more questions, feel free to let me know.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@NoahDragon can you please also help review the change? I think the disableNunjucks property is reviewed by you.

@JLHwung

This comment has been minimized.

Copy link
Collaborator

commented Jun 9, 2019

Hi think-in-universe,

Sorry for the late reply. Could you add a unit test on the new behavior? It would help people evaluate the impact of behavior change.

Thank you.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

@JLHwung, thanks for the suggestions.

Do you know where we can find the unit tests for the PR (#2593) that introduced the disableNunjucks property?

I have several tests in https://github.com/think-in-universe/hexo-stop-tag-plugins, can we just reuse that tests and move some of them to hexo's unit tests?

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Hi @be5invis,

Could you please also help review the change? I think disableNunjucks was added by you.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

For reviewers, I think this repo about youki is the place where @be5invis used the disableNunjucks property after the PR #2593 .

I didn't actually see much use case that used the disableNunjucks property, after it's introduced, though it's a common issue as illustrated above.

@be5invis

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

@think-in-universe
Markdown features are handled by Nunjucks?

@be5invis

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Also, disableNunjucks is specified in the renderer, so normal Markdown should not set this.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@be5invis one of the issue we have with PR #2593, is that the code blocks in markdown are not rendered correctly with disableNunjucks set at renderer level.

You may check hexo-stop-tag-plugins as an example.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Different to the youki use case, where you don't use any markdown I think. @be5invis

My and quite some others' use cases (as I shared above) are conflicted with nunjucks when writing markdowns.

@be5invis

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I don't think there's any problem with this PR.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

btw, @be5invis was there any unit tests after you added the disableNunjucks property for renderer?

and for documentation, @JLHwung, and @tomap where should we let hexo developers know the existence of this property, instead of searching around GitHub issues and PRs...

@be5invis

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I don't think there's tests for that.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@be5invis thanks for your quick response and review. 👍

then we may add some tests in this PR, if @JLHwung can help check the tests in https://github.com/think-in-universe/hexo-stop-tag-plugins are feasible.

@curbengh

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

the tests shown in hexo-stop-tag-plugins looks extensive. I think incorporate them into test/scripts/hexo/post.js (since disableNunjucks property is introduced in lib/hexo/post.js) would help reviewers.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@weyusi @JLHwung @tomap

Hi reviewer, please help review the change and the newly added unit test cases.

The "line coverage" decreased a bit. (I don't actually understand why. Is it because of newly added tests?) could you please also check whether the branch coverage decreased?

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Another question: if the PR is merged, which release of hexo will include this change?

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Any feedback or suggestions? @weyusi @JLHwung @tomap

@curbengh

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

The relevant line seems to be covered. I don't think other coverage issues are related to this PR.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

@weyusi , thanks.

Anything else do I need to do to make the PR pass the review? and what's the expected release to include this PR?

@JLHwung

This comment has been minimized.

Copy link
Collaborator

commented Jun 16, 2019

Look good to ship. The coverage decrease should be false positive.

@JLHwung JLHwung merged commit 9d09d2e into hexojs:master Jun 16, 2019

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.1%) to 97.148%
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@JLHwung thanks. just to check again: any ideas about the release schedule about this merge?

I'm asking because the NPM package hexo-stop-tag-plugins will depends on this PR. I'd like to specify the hexo dependency.

@JLHwung

This comment has been minimized.

Copy link
Collaborator

commented Jun 16, 2019

@think-in-universe 3.9.0 is published. Thanks for your contribution.

@think-in-universe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@JLHwung good to know that. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.