yuin / goldmark

:trophy: A markdown parser written in Go. Easy to extend, standard(CommonMark) compliant, well structured.
MIT License
3.68k stars 255 forks source link

BaseBlock.Text changed in v1.7.5 #470

Closed bep closed 3 weeks ago

bep commented 3 weeks ago

Hugo has some tests failing when upgrading to v1.7.6 that suggest that there are now a trailing newline when calling .Text on e.g. *ast.Heading that was not there before.

I assume this is e44645afbb6814bb93536587a5d4af43ad3ab2ef

Old implementation:

// Text implements Node.Text  .
func (n *BaseNode) Text(source []byte) []byte {
    var buf bytes.Buffer
    for c := n.firstChild; c != nil; c = c.NextSibling() {
        buf.Write(c.Text(source))
    }
    return buf.Bytes()
}

New implementation:

// Text implements Node.Text.
func (b *BaseBlock) Text(source []byte) []byte {
    var buf bytes.Buffer
    for _, line := range b.Lines().Sliced(0, b.Lines().Len()) {
        buf.Write(line.Value(source))
    }
    return buf.Bytes()
}
yuin commented 3 weeks ago

Not directory related with e44645afbb6814bb93536587a5d4af43ad3ab2ef. It seems a side effect of 41273a4d0725cd0f098184e01fee4e1a2516ae87.

Honestly say, I thought `Node.Text is just a trivial stuff. I added it just because it is convenient for some Nodes(i.e.: TextNode). There are no 'correct' definition of the 'text value' of a node especially BlockNodes.

I am tempted to remove Text from Node and make it a special method for some nodes like Text and String. But goldmark is already widely used. This breaking change may have significant effects for many projects.

I have tried to implement Node.Text as 'reasonable' as possible. And added warning to the GoDoc.

bep commented 3 weeks ago

There are no 'correct' definition of the 'text value' of a node especially BlockNodes.

This issue is about stability and not about correctness.

yuin commented 3 weeks ago

stability and correctness are connected in practice. If there is a function that has bugs(including 'incorrect' result) and fixing it leads different results is unstable? Fixing bugs break any code depends on this bug.

i.e. : Go1.7 changes net.ParseIP behaviour. https://go.dev/doc/go1.17#:~:text=The%20ParseIP%20and%20ParseCIDR%20functions%20now%20reject%20IPv4%20addresses

But it was released as Go1.7 not Go2.0 .