Add table of contents support #980

saschagrunert · 2019-12-16T15:22:09Z

This adds the possibility to add a generated table of contents before
the actual markdown, which will be later used for the correct assembling
of the overall CHANGELOG-x.y.md files.

Needs #978

saschagrunert · 2019-12-22T13:29:25Z

Rebased on top of the latest master, ready for review.

justaugustus · 2020-01-06T13:04:57Z

/assign @hoegaarden @hasheddan

cpanato

/lgtm
/hold for the assigners to review as well

hasheddan

Thanks @saschagrunert! One small comment but looks good otherwise :)

cmd/release-notes/main.go

hoegaarden

All just nits, except the error hiding and the thing with the backtick in the title.

hoegaarden · 2020-01-06T14:50:48Z

pkg/notes/toc.go

+}
+
+func add(result io.StringWriter, title string, indent int, headers map[string]int) {
+	toReplace := strings.NewReplacer(


nit:

Technically, I believe we'd need to reimplement https://github.com/jch/html-pipeline/blob/master/lib/html/pipeline/toc_filter.rb#L41-L56 to be correct here. But I guess we are close enough? 🤷‍♂

toReplace is a bit of a misleading name. If we are happy not to fully / correctly implement the generation of the anchor names, we could use something like:
link := strings.NewReplacer( "!", "", "#", "", // all the other replacements "~", "", " ", "-", // move the replacement of the space also to the Replacer ).Replace(strings.ToLower(title))

Changed it as suggested. I'm not sure which use-case we do not cover with the current implementation. Is there anything we missed?

E.g. multiple successive space should be replaced to one dash, I am not sure if all the chars we remove with the replacer are equal to /[^\w\- ]/, we don't do the equivalent of EscapeUtils.escape_html and maybe other (I just skimmed both this code and the github html pipeline code) ... but as said, I think we are close enough for now.

Okay, yeah sounds reasonable to me.

pkg/notes/toc.go

hoegaarden · 2020-01-06T15:08:08Z

pkg/notes/toc.go

+			if strings.Contains(prevLine, "`") {
+				continue
+			}


Not sure what that is about, TBH. Wouldn't that skip the 2nd of two successive headings, if the 1st one had a backtick in it's title?

# This has `some backticks` ## And this heading will be skipped now?

There is no test case for that either so I am not sure about the use-case / intention for that special case.

Actually, wouldn't it break if we'd had a header line with a backtick in it?

we find a header and set prevLine to it's content, coincidentally it contains a backtick

next time around the if clause matches and we continue, prevLine still is set to the thing with the backtick

next time around the if clause matches and we continue, prevLine still is set to the thing with the backtick

...

eventually we are done scanning, but never really processed much except up until the first header with a backtick

The main purpose of this is to skip code blocks (do not generate any headers inside them, because they could contain #. :) I think we have to somehow verify if we're inside a code block by the amount of backticks we've seen. 🤔

Tracking the right amount of back ticks should fix the code block issue in a more general way, PTAL :)

I don't think this is working correctly. The current implementation would ignore a perfectly valid heading

# `markdown` solves all our problems, they said

Other issues might arise with documents like

something `````` wow ... this actually seems to be valid md # ````go # hello := `hello` # ```` `````` something else

where the number of seen backticks is even in every line. This example is contrived, however we are dealing with random user input here.

Honest questions:

How much effort do we want to spend here for correctly rendering the TOC?

How likely is it that a b0rken TOC blanks out or scrambles the rest of the document?

The first example should be covered by the current implementation, I verified that with an additional unit test:

release/pkg/notes/toc_test.go

Lines 148 to 152 in 5f80175

func TestGenerateTOCBackTickInHeading(t *testing.T) {

toc, err := GenerateTOC("# `markdown` solves all our problems, they said")

require.Nil(t, err)

require.Equal(t, toc, "- [`markdown` solves all our problems, they said](#markdown-solves-all-our-problems-they-said)\n")

}

The second example is tricky, right. So what do you prefer? I could remove the overall code block check vs leaving it as it currently is.

hoegaarden · 2020-01-06T15:25:38Z

pkg/notes/toc.go

+
+var headerPattern = regexp.MustCompile("^(?P<indent>#+) ?(?P<title>.+)$")
+
+func GenerateTOC(input string) string {


nit:

I feel weird implementing a (partial) Markdown parser and generator, but I checked really quick and I could not find a give-me-a-toc-for-this-markdown-doc do module ... So I guess we gotta do what we gotta do 😄

cmd/release-notes/main.go

This adds the possibility to add a generated table of contents before the actual markdown, which will be later used for the correct assembling of the overall `CHANGELOG-x.y.md` files. Signed-off-by: Sascha Grunert <[email protected]>

hoegaarden · 2020-01-07T11:19:54Z

/lgtm
/approve

k8s-ci-robot · 2020-01-07T11:20:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, hoegaarden, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/release-notes/OWNERS~~ [hoegaarden,saschagrunert]
~~pkg/notes/OWNERS~~ [hoegaarden,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hoegaarden · 2020-01-07T11:48:39Z

/unhold

k8s-ci-robot requested review from marpaia and onyiny-ang December 16, 2019 15:22

This was referenced Dec 16, 2019

[krel] Refactor existing bash scripts into go tools #918

Closed

Add markdown creation support for krel changelog #992

Merged

saschagrunert changed the title ~~WIP: Add table of contents support~~ Add table of contents support Dec 22, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 22, 2019

justaugustus added this to the v1.18 milestone Jan 6, 2020

k8s-ci-robot assigned hasheddan and hoegaarden Jan 6, 2020

cpanato approved these changes Jan 6, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 6, 2020

k8s-ci-robot assigned cpanato Jan 6, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 6, 2020

hasheddan reviewed Jan 6, 2020

View reviewed changes

cmd/release-notes/main.go Show resolved Hide resolved

hoegaarden suggested changes Jan 6, 2020

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 6, 2020

Add table of contents generation support

5f80175

This adds the possibility to add a generated table of contents before the actual markdown, which will be later used for the correct assembling of the overall `CHANGELOG-x.y.md` files. Signed-off-by: Sascha Grunert <[email protected]>

hoegaarden approved these changes Jan 7, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2020

k8s-ci-robot merged commit 484d258 into kubernetes:master Jan 7, 2020

saschagrunert deleted the toc branch January 7, 2020 11:57

	func TestGenerateTOCBackTickInHeading(t *testing.T) {
	toc, err := GenerateTOC("# `markdown` solves all our problems, they said")
	require.Nil(t, err)
	require.Equal(t, toc, "- [`markdown` solves all our problems, they said](#markdown-solves-all-our-problems-they-said)\n")
	}


		var headerPattern = regexp.MustCompile("^(?P<indent>#+) ?(?P<title>.+)$")

		func GenerateTOC(input string) string {

Add table of contents support #980

Add table of contents support #980

Uh oh!

Conversation

saschagrunert commented Dec 16, 2019

Uh oh!

saschagrunert commented Dec 22, 2019

Uh oh!

justaugustus commented Jan 6, 2020

Uh oh!

cpanato left a comment

Choose a reason for hiding this comment

Uh oh!

hasheddan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoegaarden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hoegaarden commented Jan 7, 2020

Uh oh!

k8s-ci-robot commented Jan 7, 2020

Uh oh!

hoegaarden commented Jan 7, 2020

Uh oh!

Uh oh!