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

x/pkgsite: consider moving to goldmark to handle issues in READMEs #39297

Closed
hhrutter opened this issue May 28, 2020 · 6 comments
Closed

x/pkgsite: consider moving to goldmark to handle issues in READMEs #39297

hhrutter opened this issue May 28, 2020 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite

Comments

@hhrutter
Copy link

What is the URL of the page with the issue?

https://pkg.go.dev/mod/github.com/pdfcpu/pdfcpu

What is your user agent?

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.138 Safari/537.36

Some images don't show up and there is also a markdown table that's screwed up.

@gopherbot gopherbot added this to the Unreleased milestone May 28, 2020
@julieqiu
Copy link
Member

Thanks for the feedback @hhrutter - we'll work on a fix! This is also related to #37194

@julieqiu julieqiu added the NeedsFix The path to resolution is known, but the work has not been done. label May 28, 2020
@julieqiu julieqiu changed the title go.dev: readme.md not rendered properly go.dev: broken image links and markdown table in README for github.com/pdfcpu/pdfcpu May 28, 2020
@julieqiu julieqiu changed the title go.dev: broken image links and markdown table in README for github.com/pdfcpu/pdfcpu x/pkgsite: broken image links and markdown table in README for github.com/pdfcpu/pdfcpu Jun 15, 2020
@josh-newman
Copy link

In case it's helpful, I ran into another case of incorrect table rendering, this time with no images in the table: correct on Github (snapshot), incorrect on pkg.go.dev (snapshot).

@shaqque
Copy link
Contributor

shaqque commented Aug 5, 2020

The broken markdown tables for both packages (pdfcpu and parquet-go) seem to be caused by incorrectly formatted markdown, at least according to blackfriday (the external markdown parser pkg.go.dev uses). Note that Github-flavored-markdown (GFM), which Github uses, is not the same as regular markdown.

Specifically,

  1. pdfcpu's table does not does not have a space/newline before the start of the table, causing blackfriday to not detect it as a table. Adding that space allows blackfriday to detect and render the table correctly.
  2. parquet-go's table uses 1 hyphen per column in the header row instead of 3, which blackfriday requires (see Help getting markdown table working russross/blackfriday#351). This also appears to violate Github's documentation for tables, though the official Github-flavored-markdown spec doesn't say anything about this despite the examples all having 3 hyphens per column. Changing the table to use 3 hyphens per column in the header row allows blackfriday to detect and render the table correctly.

There is a related discussion on #40203 about blackfriday and GFM. In particular, @zikaeroh suggested

Unless I'm mistaken, blackfriday isn't actively developed anymore, and many have moved onto goldmark. Hugo, gitea, and even x/tools/present and x/website use goldmark for their markdown support.

Perhaps it'd be better to move to a markdown library which is actively maintained.

I tried the above cases using goldmark, and in both cases, the table was identified and rendered properly by goldmark.

I think this, along with active maintenance (last commit was 6 days ago) and broader use among other Go projects, makes moving to goldmark a good idea.

@julieqiu julieqiu changed the title x/pkgsite: broken image links and markdown table in README for github.com/pdfcpu/pdfcpu x/pkgsite: consider moving to goldmark to issues in READMEs Aug 13, 2020
@julieqiu julieqiu added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Aug 13, 2020
@julieqiu julieqiu changed the title x/pkgsite: consider moving to goldmark to issues in READMEs x/pkgsite: consider moving to goldmark to handle issues in READMEs Aug 19, 2020
@gopherbot
Copy link

Change https://golang.org/cl/266579 mentions this issue: content/static: copy readme.css into legacy_readme.css

gopherbot pushed a commit to golang/pkgsite that referenced this issue Nov 2, 2020
Code in motion. Preparing to generate new readme styles for
the goldmark parser.

For golang/go#39297

Change-Id: I506046ba36fdef6fa935b0600e1037e2d832ad24
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/266579
Trust: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/267117 mentions this issue: internal/frontend: separate goldmark readme code from legacy overview code

gopherbot pushed a commit to golang/pkgsite that referenced this issue Nov 3, 2020
… code

The overview file contains mostly legacy code used
to construct the overview page. This change separates
the goldmark code in preparation for updates that will
generate a TOC for the readme and the future removal of
all overview related code.

For golang/go#39297

Change-Id: Ifa5d0ee3983478fd25c6c59fc1bd2c45457cc05c
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/267117
Run-TryBot: Jamal Carvalho <jamal@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Trust: Jamal Carvalho <jamal@golang.org>
@jamalc jamalc self-assigned this Nov 18, 2020
@jamalc
Copy link

jamalc commented Nov 18, 2020

@hhrutter - this should be fixed. We made the switch to goldmark 😊

@jamalc jamalc closed this as completed Nov 18, 2020
@golang golang locked and limited conversation to collaborators Nov 18, 2021
@rsc rsc unassigned jamalc Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Projects
None yet
Development

No branches or pull requests

6 participants