osteele / gojekyll

A fast Go implementation of the Jekyll blogging engine
MIT License
332 stars 14 forks source link

add support for go mod and blackfriday v2 #39

Closed mevdschee closed 3 years ago

osteele commented 3 years ago

Thank you for this! I have a couple of questions. I've also made some changes to the main repo. They don't introduce any merge conflicts, but If you're doing something in the go.mod file that answers the second question, that will show up as a diff if you rebase.

  1. go.sum My reading of https://github.com/golang/go/wiki/Modules#releasing-modules-all-versions is that go.sum should go in the repo (if go.mod does). That seems like a good idea to make sure that all developers and the CI are using the same versions of dependent modules.

  2. I was unable to run your fork. I get an error:

    go test ./...
    # github.com/osteele/gojekyll/filters
    filters/filters.go:90:34: undefined: blackfriday.Run
    # github.com/osteele/gojekyll/filters [github.com/osteele/gojekyll/filters.test]
    filters/filters.go:90:34: undefined: blackfriday.Run

    I was able to resolve error this by changing "github.com/russross/blackfriday" to "github.com/russross/blackfriday/v2" in filters/filters.go and renderers/markdown.go. Since your code didn't include this change, it looks like you had a different way of resolving this, though – maybe through some syntax in go.mod? [Edit: I just read about the replace directive. Is this what you used?]

mevdschee commented 3 years ago

Thank you for creating the software and for taking the time to look at this merge request

That seems like a good idea to make sure that all developers and the CI are using the same versions of dependent modules.

I think you are right: I should have included both go.mod and go.sum.

maybe through some syntax in go.mod?

Yes I think I settled on the v2 version of blackfriday as a requirement in go.mod.

NB: I'm totally fine with you continuing the work on your branch and closing this merge request (I hope it helped in some way).

osteele commented 3 years ago

Thank you for the contribution – it is helping, and I look forward to being able to upgrade to Blackfriday v2.

Can you share your go.mod file? I would like to see if I'm missing something from it.

Running the tests, I get an error in markdown_test.go:

ok      github.com/osteele/gojekyll/plugins     (cached)
--- FAIL: TestRenderMarkdown (0.00s)
    markdown_test.go:21: 
                Error Trace:    markdown_test.go:21
                Error:          "<p><div><p><a href=\"mailto:user@example.com\">user@example.com</p>\n</a></div></p>\n" does not contain "<a href=\"mailto:user@example.com\">user@example.com</a>"
                Test:           TestRenderMarkdown
FAIL

It looks like all these calls in markdown_test.go insert an extra <p>…</p> in the output:

    require.Equal(t, "<div>*b*</div>\n", mustMarkdownString("<div>*b*</div>"))
    require.Equal(t, "<div a=1><p><em>b</em></p>\n</div>\n", mustMarkdownString(`<div a=1 markdown="1">*b*</div>`))
    require.Equal(t, "<div a=1><p><em>b</em></p>\n</div>\n", mustMarkdownString(`<div a=1 markdown='1'>*b*</div>`))
    require.Equal(t, "<div a=1><p><em>b</em></p>\n</div>\n", mustMarkdownString(`<div a=1 markdown=1>*b*</div>`))

Is there a Blackfriday flag that controls this?

Or…perhaps it is not consequential? (Perhaps the test is overly specific.) I haven't done the research on whether this affects actual site rendering, or just this test.

This test also fails. It generates an error. If Blackfriday v2 doesb't accept markdown=1 in order to render embedded markdown, this could be an issue. Is there an additional flag for this?

    _, err := renderMarkdownString(`<div a=1 markdown=1><p></div>`)
    require.NotNil(t, err)
    require.Contains(t, err.Error(), "EOF")
osteele commented 3 years ago

Generated output from a markdown page in a test site looks good. It is just the test suite that is in error.

I'll fix the tests and Skip the one that I don't know how to fix, and merge this now. Again, thank you for the contribution.

mevdschee commented 3 years ago

Thank you for your kind words, I'm glad it is helpful.

it is helping, and I look forward to being able to upgrade to Blackfriday v2. Can you share your go.mod file? I would like to see if I'm missing something from it.

I just pulled your file and since I'm on Go 1.13 (Ubuntu 20.04) I can't compile as you use "io/fs" which was introduced in 1.16

build github.com/osteele/gojekyll: cannot load io/fs: malformed module path "io/fs": missing dot in first path element

It looks like all these calls in markdown_test.go insert an extra <p>…</p> in the output:

I noticed this and I don't know a way around it. Maybe Goldmark? I know Hugo switched to this library. It may make sense.

In the goldmark readme is stated:

blackfriday.v2 is a fast and widely-used implementation, but is not CommonMark-compliant and cannot be extended from outside of the package, since its AST uses structs instead of interfaces.

I'm not sure how this would really impact gojekyll though.

This test also fails. It generates an error. If Blackfriday v2 doesn't accept markdown=1 in order to render embedded markdown, this could be an issue. Is there an additional flag for this?

I'm not aware of such flag.