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

add support for go mod and blackfriday v2 #39

Merged
merged 2 commits into from Jun 19, 2021
Merged

add support for go mod and blackfriday v2 #39

merged 2 commits into from Jun 19, 2021

Conversation

mevdschee
Copy link
Contributor

No description provided.

@mevdschee mevdschee changed the title add support for go mod add support for go mod and blackfriday v2 Jun 4, 2021
@osteele osteele self-assigned this Jun 16, 2021
@osteele
Copy link
Owner

osteele commented Jun 16, 2021

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
Copy link
Contributor Author

mevdschee commented Jun 16, 2021

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
Copy link
Owner

osteele commented Jun 18, 2021

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
Copy link
Owner

osteele commented Jun 18, 2021

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
Copy link
Contributor Author

mevdschee commented Jun 18, 2021

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.

@osteele osteele merged commit 13c4832 into osteele:master Jun 19, 2021
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.

None yet

2 participants