Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tpl/transform: Avoid removing p tags in markdownify when marked as blocks #3786

Closed
wants to merge 2 commits into from

Conversation

@bep
Copy link
Member

commented Aug 8, 2017

As in the example below:

{{ "Blocks of *text*." | blocks | markdownify}}

Fixes #3040

Note: The block keyword is reserved.

@bep bep changed the title tpl/transform: Avoid removing p tags in markdownify when marked as blocks As in the example below: tpl/transform: Avoid removing p tags in markdownify when marked as blocks Aug 8, 2017
@bep bep requested a review from moorereason Aug 8, 2017
@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2017

Naming is hard; I have introduced a new keyword (or function), blocks ... The idea is good, I think, as it avoids having to create new template func variant for all options. But it could maybe be called blockify... /cc @digitalcraftsman

@digitalcraftsman

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

There are only two hard things in Computer Science: cache invalidation and naming things.

— Phil Karlton

The only senseful alternative that pops up in my mind would be paragraphify. blockify with the ify suffix would be in line with other content transformers and with your code.

I'm fine with both unless someone has an alternative.

@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2017

The only senseful alternative that pops up in my mind would be paragraphify.

The problem is that in this case blocks mean "do not remove any paragraphs".

bep added 2 commits Aug 8, 2017
To mark som text as one or more blocks. Note that the `block` keyword is reserved in Go templates, hence this plural.

See #3040
…ocks

As in the example below:

```
{{ "Blocks of *text*." | blocks | markdownify}}
```

Fixes #3040
@bep bep force-pushed the bep:markdownify-block branch from 6ab21c2 to a3ac9fe Aug 8, 2017
@digitalcraftsman

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

Thanks for the clarification. What about keepParagraphs?

@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2017

Thanks for the clarification. What about keepParagraphs?

No, I think that is too restrictive -- this is a term that describes a text as "something"; that we in this particular case "keep paragraphs" is just an implementation detail. It is an option and a type (simliar to int and string).

@digitalcraftsman

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

That's a tough question. Do some of our native English speaking friends know a good alternative?

/cc @rdwatters, @moorereason, @anthonyfok

@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2017

How about prose:

{{ "Blocks of *text*." | prose | markdownify}}

Or body:

{{ "Blocks of *text*." | body | markdownify}}

Or maybe just ... blocks ... or textBlock...

@frankspin89

This comment has been minimized.

Copy link

commented Aug 8, 2017

I don't know if tags is a reserved word?

''' tags l markdownify '''

Or

''' HTML | markdownify '''

Last suggestion

''' raw | markdownify '''

@adiabatic

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

Something with an as prefix?

  • asParagraph
  • asBlock

…etc.

Copy link
Contributor

left a comment

This is a simple way to work around the quirks in markdownify. I would call it blockify instead of blocks. Other than that, the PR looks great.

However, I'm struggling with the whole idea of this PR. I understand the use case and why we are where we are, but I don't really like the seemingly magical "paragraph trim" behavior of markdownify. Most people probably don't even know it's happening, but when it breaks your perfectly legitimate markdown content, I'm sure the user thinks they've found a bug and might wonder if we know what we're doing.

Two other options come to mind:

  1. Come at it from the other direction by providing a strings.TrimXML func (or just use strings.TrimPrefix and strings.TrimSuffix but that gets tedious).
{{ "Blocks of *text*." | markdownify | trimXML "p" }}
  1. Another magical but more advanced option: parse the generated HTML from markdownify (https://github.com/tdewolff/parse), and see if we have multiple blocks. If so, leave the p tags alone.

In the end, adding this blocks quirk func isn't the end of the world. It just doesn't seem very elegant. Anyway, I've said my piece. 😁

@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

Come at it from the other direction by providing a strings.TrimXML f

@moorereason are you willing to take all the support work of such a change?

I created the markdownify func and

  1. Its main use case was and is to use on short snippets of text in titles etc., and to my best knowledge this is not valid HTML:
<h1><p>Foo!</p></h1>
  1. If we're going to change the semantics of markdownify then we break a lots of sites (mine included) in the wild.

We're not parsing either, but if a count("p>") > 2 would be good enough, that could be considered - but that sounds kind of shaky...

@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

I have created an alternative fix in #3789

Up to you:


@bep

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2017

A clear winner ...

@bep bep closed this Aug 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.