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

Why do AST methods have `self`? #105

Closed karelbilek closed 4 years ago

karelbilek commented 4 years ago

I cannot understand the self argument in all AST methods.

It's almost never used, and not at all documented. Why is it

InsertBefore(self, v1, insertee Node)

and not just

InsertBefore(v1, insertee Node)

? (The same with other methods)

karelbilek commented 4 years ago

By looking around code, I cannot find a case when self is not the same as the Node object. That is, it's always repeated, like so

node.Parent().RemoveChild(node.Parent(), node)

I understand that currently the API cannot be changed without breaking backwards comp; but maybe it should be "deprecated" (that is, self should not be used) and documented?

karelbilek commented 4 years ago

Oh, it's because you use embedding, and therefore n and self are different? (n is BaseNode, self is Node?)

Hm, I feel like it's an antipattern in some way.... I cannot put my finger on how to do it correctly though

anyway it cannot really be changed now without breaking compat, I get that. But it feels anti-pattern-y to me :D

yuin commented 4 years ago

Oh, it's because you use embedding, and therefore n and self are different? (n is BaseNode, self is Node?)

Yes, It is.

karelbilek commented 4 years ago

I feel like this is a misuse of embedding to do false "inheritance", but whatever works :)

Btw thanks for this project, it's amazing

myitcv commented 4 years ago

One way of dealing with this would have been to code generate methods on types that transitively embed BaseNode, and have them call a non-exported version on BaseNode. e.g. one such generated method would be:

func (n *HTMLBlock) AppendChild(child Node) {
    return n.appendChild(n, child)
}

Then have the following method defined:


func (n *BaseNode) appendChild(self, v Node) {
    // ... current implementation
}
karelbilek commented 4 years ago

It would also break all plugins, so not worth it I think, even when it's a bit "wtf" at first

myitcv commented 4 years ago

It would also break all plugins

Yes, making such a change is clearly not backwards compatible, so would be something to consider for v2.