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

chore(package): update marked 0.3.9 to 0.6.1 #87

Merged
merged 6 commits into from
Apr 6, 2019
Merged

chore(package): update marked 0.3.9 to 0.6.1 #87

merged 6 commits into from
Apr 6, 2019

Conversation

yoshinorin
Copy link
Member

Details

Update marked from 0.3.9 to 0.6.0 & fixed testcode for breaking change in marked v0.4.0

@coveralls
Copy link

coveralls commented Feb 23, 2019

Coverage Status

Coverage decreased (-1.6%) to 87.5% when pulling f81890c on YoshinoriN:marked-v0.6.0 into 88d39fb on hexojs:master.

@tomap
Copy link
Contributor

tomap commented Feb 23, 2019

I would like to test changes in rendering for both cases: lists and checkbox, before validating. It would deserve a minor or major version bump. And we should check the rendering of our default theme

@yoshinorin
Copy link
Member Author

I would like to test changes in rendering for both cases: lists and checkbox, before validating.

OK. I will try it later.

And we should check the rendering of our default theme

OK. It's better. But, how to do? Manually & visually? I have no idea...

@tomap
Copy link
Contributor

tomap commented Feb 23, 2019

I'll try to do it manually this week end. I'll setup a test site

@yoshinorin
Copy link
Member Author

OK. May I help you?

@tomap
Copy link
Contributor

tomap commented Feb 24, 2019

Here is the result before:
https://hexo-theme-minidyne-demo.netlify.com/2013/12/24/elements/

And after:
https://5c72c73a8e0d8f0008bc21d4--hexo-theme-minidyne-demo.netlify.com/2013/12/24/elements/

I'd say it improved! checkboxes unchecked are working.
The only change is that those checkboxes are disabled, but that seems to be the definition of gfm:
https://github.github.com/gfm/#task-list-items-extension-
I believe our code is no longer necessary. I tested without, and it works fine

Also, I tested with commenting the autolink feature, and it still works

The heading code seems to still be necessary, same for Description Lists.

You should adapt code and drop tests unnecessary

So considering all this, we definitely should bump the major version (changes in rendering)

It will also allow us to close a bunch of checkbox related PR/Issues

@tomap tomap mentioned this pull request Feb 24, 2019
@yoshinorin
Copy link
Member Author

I bump to marked 0.6.1 & drop self implemented todo list.

@amejiarosario
Copy link

amejiarosario commented Feb 27, 2019

I just noticed this branch fixes the #86. However, it breaks the tables!

@yoshinorin Should we add unit tests for this tables so they don't break on future updates?

Name | Insert | Access | Search | Delete | Comments
-|-|-|-|-
[**Array**](#Array) | [*O(n)*](#Insert-element-on-an-array) | [*O(1)*](#Access-an-element-in-an-array) | [*O(n)*](#Search-an-element-in-an-array) | [*O(n)*](#Deleting-elements-from-an-array) | Insertion to the end is `O(1)`. [Details here.](#Array-operations-time-complexity)
[(Hash)**Map**](#HashMaps) | [*O(1)**](#Insert-element-on-a-HashMap-runtime) | [*O(1)**](#Search-Access-an-element-on-a-HashMap-runtime) | [*O(1)**]

Rendering with Marked 0.6

image

Before this table used look like this:

Rendering with Marked 0.3

image

@tomap
Copy link
Contributor

tomap commented Feb 27, 2019

@amejiarosario It seems related to a change in release 0.4:
https://github.com/markedjs/marked/releases/tag/0.4.0
You can test here:
https://marked.js.org/demo/?text=Name%20%7C%20Insert%20%7C%20Access%20%7C%20Search%20%7C%20Delete%20%7C%20Comments%0A-%7C-%7C-%7C-%7C-%0A%5B**Array**%5D(%23Array)%20%7C%20%5B*O(n)*%5D(%23Insert-element-on-an-array)%20%7C%20%5B*O(1)*%5D(%23Access-an-element-in-an-array)%20%7C%20%5B*O(n)*%5D(%23Search-an-element-in-an-array)%20%7C%20%5B*O(n)*%5D(%23Deleting-elements-from-an-array)%20%7C%20Insertion%20to%20the%20end%20is%20%60O(1)%60.%20%5BDetails%20here.%5D(%23Array-operations-time-complexity)%0A%5B(Hash)**Map**%5D(%23HashMaps)%20%7C%20%5B*O(1)**%5D(%23Insert-element-on-a-HashMap-runtime)%20%7C%20%5B*O(1)**%5D(%23Search-Access-an-element-on-a-HashMap-runtime)%20%7C%20%5B*O(1)**%5D&options=%7B%0A%20%22baseUrl%22%3A%20null%2C%0A%20%22breaks%22%3A%20true%2C%0A%20%22gfm%22%3A%20true%2C%0A%20%22headerIds%22%3A%20true%2C%0A%20%22headerPrefix%22%3A%20%22%22%2C%0A%20%22highlight%22%3A%20null%2C%0A%20%22langPrefix%22%3A%20%22language-%22%2C%0A%20%22mangle%22%3A%20true%2C%0A%20%22pedantic%22%3A%20true%2C%0A%20%22sanitize%22%3A%20true%2C%0A%20%22sanitizer%22%3A%20null%2C%0A%20%22silent%22%3A%20false%2C%0A%20%22smartLists%22%3A%20true%2C%0A%20%22smartypants%22%3A%20true%2C%0A%20%22tables%22%3A%20true%2C%0A%20%22xhtml%22%3A%20false%0A%7D&version=0.3.19

change the version from 0.3.19 to 0.4, you'll see that it changes the rendering of tables
You'll need to add an initial pipe to fix this
=> @yoshinorin We definitely need to bump major version for this one

@yoshinorin
Copy link
Member Author

@amejiarosario
Thank you for your advise :) But, as @tomap say it seems breaking change of marked. Perhaps we can't deal with it.

@tomap
Yes. We should update major version. But, I prefer divided it into other PR.

@amejiarosario
Copy link

@yoshinorin the breaking change is that marked is less forgiving. My table was missing a |- and that's why was not rendering. After fixing that is working on marked 0.6.x. So, it should be ok to migrate to this version.

@tomap
Copy link
Contributor

tomap commented Mar 15, 2019

@yoshinorin the coverage decreased
You should be able to increase it back by adding a test similar to the autolink test

describe('autolink option tests', function() {

and cover the case of sanatize that is not covered

Copy link
Contributor

@tomap tomap left a comment

Choose a reason for hiding this comment

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

Increasing the coverage would be nice, but appart from that, I validate

@yoshinorin yoshinorin changed the title Marked v0.6.0 chore(package): update marked 0.3.9 to 0.6.1 Mar 18, 2019
@yoshinorin
Copy link
Member Author

@tomap Thank you review :)

Coverage decrease is caused by delete source code (Not include test code). This PR delete self-implemented TODO-list.

My understnding, the coveralls calculate coverage based on how many lines of the total code lines were tested. So, If source code delete, coverage will be decrease.

@yoshinorin
Copy link
Member Author

@tomap

I validate

Is this means... Will you validate this PR?
May I merge this?

@yoshinorin
Copy link
Member Author

@tomap
I'm sorry ask again. What is mean of I validate?
May I merge this? This upgrade will fix some issues. IMHO we should merge this.

Thanks :)

@tomap
Copy link
Contributor

tomap commented Apr 6, 2019

I think we should merge

@yoshinorin yoshinorin merged commit 3317aeb into hexojs:master Apr 6, 2019
@yoshinorin yoshinorin deleted the marked-v0.6.0 branch April 6, 2019 14:33
@yoshinorin
Copy link
Member Author

Merged Thanks :)

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.

4 participants