search

russross / blackfriday

Blackfriday: a markdown processor for Go
Other
5.43k stars 598 forks source link

v2: API enhancement: add ListTypeUnordered to enum #367

Open client9 opened 7 years ago

client9 commented 7 years ago

Hello!

I'm hacking on V2 and loving the AST. I'll post some other issues about it in separate tickets.

One nit while working on a renderer and wanted to know directly if a list was UNordered or not. Given the current enum/const/flag:

// These are the possible flag values for the ListItem renderer.
// Multiple flag values may be ORed together.
// These are mostly of interest if you are writing a new output format.
const (
    ListTypeOrdered ListType = 1 << iota
    ListTypeDefinition
    ListTypeTerm

    ListItemContainsBlock
    ListItemBeginningOfList // TODO: figure out if this is of any use now
    ListItemEndOfList
)

It's easy to see if something is ordered flag&ListTypeOrdered != 0 (or other variations), but checked to see if something is unordered is a really ugly and fragile (I'll leave as an exercise to the reader). (Actually don't know what ListTypeTerm is ... yet).

I suggest adding ListTypeUnordered to be explicit and to make the API consistent and robust against future changes. I don't think it will change the HTML rendered at all.

I'm happy to do a PR, but there may be history or other reasons to keep things the as they now. And it's entirely possible I'm missing how to do this in another way.

thanks for a great project.

n

rtfb commented 7 years ago

The historical reason for not having a constant for unordered list is that v1 didn't have it. Given that, I'm pretty sure 0 was meant to mean an unordered list, but then other flags grew on top and that is no longer as simple as intended.

The proposal to have it named explicitly sounds good to me. Go ahead with the PR if you're up to it. It might be a good opportunity to revisit this bit: // Multiple flag values may be ORed together.. I don't like this ancient practice and if we can get rid of it, the merrier.

ListTypeTerm is about definition lists. It distinguishes the defined term from definition.

client9 commented 7 years ago

Great, I'll take a look and see about a PR.

I also found that Tight doesn't seem to be used anymore (good.. the current v2 HTML output is closer to what GitHub uses)