Kunde21 / markdownfmt

Like gofmt, but for Markdown.
MIT License
55 stars 7 forks source link

Cleanup Renderer implementation #16

Closed bwplotka closed 3 years ago

bwplotka commented 3 years ago

1, There are some TODOs still in code:

// TODO: Clean these up.
    headers      []string
    columnAligns []extAST.Alignment
    columnWidths []int
    cells        []string

As well as minor inconsistencies.

  1. For example, buf usage is quite ambiguous. At some point, we hold so many of those and I am not sure if there is a strong reason. (e.g why we cannot use just resBuf instead of mr.buf). The problem with mr.buf is that this variable is suddenly used differently depending on the context which can lead easily to side effects etc. It would be nice to scope buf only to mr.RenderSingle to ensure we can e.g render things concurrently at some point!
func (mr *Renderer) renderChildren(source []byte, node ast.Node) []byte {
    oldBuf := mr.buf
    mr.buf = bytes.NewBuffer(nil)
    mr.normalTextMarker = map[*bytes.Buffer]int{}
    resBuf := bytes.NewBuffer(nil)
    for n := node.FirstChild(); n != nil; n = n.NextSibling() {
        ast.Walk(n, func(n ast.Node, entering bool) (ast.WalkStatus, error) {
            return mr.RenderSingle(resBuf, source, n, entering), nil
        })
    }
    resBuf.Write(mr.buf.Bytes())
    mr.buf = oldBuf
    return resBuf.Bytes()
}
  1. Is there any reason RenderSingle is public?
karelbilek commented 3 years ago

oh the whole thing is very messy :P at one point we use byte.buffer pointer as key in map...

the thing is, all this was originally meant to be a short time hack for our usage and later cleaned up, which we never did in the end :D

karelbilek commented 3 years ago

yeah these buffers are kind of nonsensical, I think it's a remnant of some old design

karelbilek commented 3 years ago

RenderSingle is public, because we use it in our code somewhere else outside of this project, I think.

bwplotka commented 3 years ago

Understandable =D

Kunde21 commented 3 years ago

Bit of a cruft double-whammy.
I migrated about 80% to blackfriday v2 before we ran into more of the inconsistencies there, then @karelbilek migrated that about 90% to goldmark.
Getting to the point where I want to go over it with a fine-tooth comb and refactor/remove the remnants, when I have the time (which is probably going to be next month, at the earliest)

bwplotka commented 3 years ago

I actually went ahead and cleaned all. Sorry for lots of changes, hope it's reviewable and not biased :crossed_fingers:

https://github.com/Kunde21/markdownfmt/pull/17