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

shortcode output wrapped in p-tags #1148

Closed
bep opened this Issue May 18, 2015 · 20 comments

Comments

Projects
None yet
7 participants

@bep bep self-assigned this May 18, 2015

@bep bep added the Bug label May 18, 2015

@bep bep added this to the v0.14 milestone May 18, 2015

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep May 18, 2015

Member

I remember adding divs around the shorcode placeholders to prevent this, but this was modified by @halostatue to fix an issue with the relref shortcodes. I guess we can remove the superflous p tags when doing the replacement.

Member

bep commented May 18, 2015

I remember adding divs around the shorcode placeholders to prevent this, but this was modified by @halostatue to fix an issue with the relref shortcodes. I guess we can remove the superflous p tags when doing the replacement.

@halostatue

This comment has been minimized.

Show comment
Hide comment
@halostatue

halostatue May 18, 2015

Contributor

👍

Contributor

halostatue commented May 18, 2015

👍

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep May 19, 2015

Member

Yea, well, a expansion of the regexp in the last replace func is a 15% slow-down on my not very shortcode-heavy site.

For the shortcodereplacement alone:

BenchmarkReplaceShortcodeTokens     916019        1714125       +87.13%

Back to the thinking box.

Member

bep commented May 19, 2015

Yea, well, a expansion of the regexp in the last replace func is a 15% slow-down on my not very shortcode-heavy site.

For the shortcodereplacement alone:

BenchmarkReplaceShortcodeTokens     916019        1714125       +87.13%

Back to the thinking box.

@darylteo

This comment has been minimized.

Show comment
Hide comment
@darylteo

darylteo Jun 20, 2015

I'm trying to understand the source (not a go dev sorry), and it looks like you're processing the shortcodes and replacing them with shortcode placeholders. Perhaps if you replaced it with a html based placeholder markdown wouldn't wrap it with paragraphs tags.

For example:

func createShortcodePlaceholder(id int) string {
    return fmt.Sprintf("<div %s=\"%d\"></div>", shortcodePlaceholderPrefix, id)
}

Anyway, fixing this would be really handy, although it isn't a "end of the world" issue I suppose. I'm working around this by putting div tags around the shortcode on the content itself.

darylteo commented Jun 20, 2015

I'm trying to understand the source (not a go dev sorry), and it looks like you're processing the shortcodes and replacing them with shortcode placeholders. Perhaps if you replaced it with a html based placeholder markdown wouldn't wrap it with paragraphs tags.

For example:

func createShortcodePlaceholder(id int) string {
    return fmt.Sprintf("<div %s=\"%d\"></div>", shortcodePlaceholderPrefix, id)
}

Anyway, fixing this would be really handy, although it isn't a "end of the world" issue I suppose. I'm working around this by putting div tags around the shortcode on the content itself.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jun 20, 2015

Member

@darylteo you are correct, that was my initial approach. But @halostatue changed that as it had some other side effects.

Member

bep commented Jun 20, 2015

@darylteo you are correct, that was my initial approach. But @halostatue changed that as it had some other side effects.

@bep bep modified the milestones: v0.15, v0.14 Jun 21, 2015

bep added a commit to bep/hugo that referenced this issue Jun 21, 2015

Remove superfluous p-tags around shortcodes
This commit replaces the regexp driven `replaceShortcodeTokens` with a handwritten one.

It wasnt't possible to handle the p-tags case without breaking performance.

This fix actually improves in that area:

```
benchmark                           old ns/op     new ns/op     delta
BenchmarkParsePage                  142738        142667        -0.05%
BenchmarkReplaceShortcodeTokens     665590        575645        -13.51%
BenchmarkShortcodeLexer             176038        181074        +2.86%

benchmark                           old allocs     new allocs     delta
BenchmarkParsePage                  87             87             +0.00%
BenchmarkReplaceShortcodeTokens     9631           9424           -2.15%
BenchmarkShortcodeLexer             274            274            +0.00%

benchmark                           old bytes     new bytes     delta
BenchmarkParsePage                  141830        141830        +0.00%
BenchmarkReplaceShortcodeTokens     52275         35219         -32.63%
BenchmarkShortcodeLexer             30177         30178         +0.00%
```

Fixes #1148

@bep bep closed this in 004fcdd Jun 21, 2015

bep added a commit that referenced this issue Jun 22, 2015

Use pooled buffer in replaceShortcodes
Even as a copy at the end is needed, this consumes way less memory on Go 1.4.2:

```benchmark                           old ns/op     new ns/op     delta
BenchmarkParsePage                  145979        139964        -4.12%
BenchmarkReplaceShortcodeTokens     633574        631946        -0.26%
BenchmarkShortcodeLexer             195842        187938        -4.04%

benchmark                           old allocs     new allocs     delta
BenchmarkParsePage                  87             87             +0.00%
BenchmarkReplaceShortcodeTokens     9424           9415           -0.10%
BenchmarkShortcodeLexer             274            274            +0.00%

benchmark                           old bytes     new bytes     delta
BenchmarkParsePage                  141830        141830        +0.00%
BenchmarkReplaceShortcodeTokens     35219         25385         -27.92%
BenchmarkShortcodeLexer             30178         30177         -0.00%
```
See #1148
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jun 22, 2015

Member

@darylteo I have tested this one pretty good myself, but it would be cool if you also could take the latest source for a spin and confirm that it it's fixed for you.

Member

bep commented Jun 22, 2015

@darylteo I have tested this one pretty good myself, but it would be cool if you also could take the latest source for a spin and confirm that it it's fixed for you.

@nicolinuxfr

This comment has been minimized.

Show comment
Hide comment
@nicolinuxfr

nicolinuxfr Jun 22, 2015

Is it related in any way with <p> around images or not at all ?

nicolinuxfr commented Jun 22, 2015

Is it related in any way with <p> around images or not at all ?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jun 22, 2015

Member

@nicolinuxfr it is if you use the figure shortcode and similar.

Member

bep commented Jun 22, 2015

@nicolinuxfr it is if you use the figure shortcode and similar.

@nicolinuxfr

This comment has been minimized.

Show comment
Hide comment
@nicolinuxfr

nicolinuxfr Jun 22, 2015

@bep nope, I'm using plain Markdown to insert images and I have <p>around them. I guess I should open a new bug report then ?

nicolinuxfr commented Jun 22, 2015

@bep nope, I'm using plain Markdown to insert images and I have <p>around them. I guess I should open a new bug report then ?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jun 22, 2015

Member

@nicolinuxfr yes, but that would be for Blackfriday: https://github.com/russross/blackfriday/issues

Member

bep commented Jun 22, 2015

@nicolinuxfr yes, but that would be for Blackfriday: https://github.com/russross/blackfriday/issues

@darylteo

This comment has been minimized.

Show comment
Hide comment
@darylteo

darylteo Jun 23, 2015

@nicolinuxfr Images would be inline so they should have p's around them, I
believe.

@bep I will try. I'm installing Hugo via Brew so I'll need to figure out
how to get a custom build first =)

On Tue, Jun 23, 2015 at 5:35 AM, Bjørn Erik Pedersen <
notifications@github.com> wrote:

@nicolinuxfr https://github.com/nicolinuxfr yes, but that would be for
Blackfriday: https://github.com/russross/blackfriday/issues


Reply to this email directly or view it on GitHub
#1148 (comment).

darylteo commented Jun 23, 2015

@nicolinuxfr Images would be inline so they should have p's around them, I
believe.

@bep I will try. I'm installing Hugo via Brew so I'll need to figure out
how to get a custom build first =)

On Tue, Jun 23, 2015 at 5:35 AM, Bjørn Erik Pedersen <
notifications@github.com> wrote:

@nicolinuxfr https://github.com/nicolinuxfr yes, but that would be for
Blackfriday: https://github.com/russross/blackfriday/issues


Reply to this email directly or view it on GitHub
#1148 (comment).

@dunn

This comment has been minimized.

Show comment
Hide comment
@dunn

dunn Jun 23, 2015

Contributor

@darylteo if you just need the latest commits from the master branch, brew install --HEAD hugo

Contributor

dunn commented Jun 23, 2015

@darylteo if you just need the latest commits from the master branch, brew install --HEAD hugo

@darylteo

This comment has been minimized.

Show comment
Hide comment
@darylteo

darylteo Jun 23, 2015

@dunn wonderful

@bep confirmed working for my use cases (figure and youtube shortcodes)

darylteo commented Jun 23, 2015

@dunn wonderful

@bep confirmed working for my use cases (figure and youtube shortcodes)

@bclermont

This comment has been minimized.

Show comment
Hide comment
@bclermont

bclermont Aug 29, 2015

Contributor

Well, this isn't totally fixed.

If I use the follow shortcode shortcodes/bleh.html:

<div>
    {{ .Inner }}
</div>

that way:

{{% bleh %}}
Markdown content
{{% /bleh %}}

it render like this:

<div>
Markdown content
<p></div></p>
Contributor

bclermont commented Aug 29, 2015

Well, this isn't totally fixed.

If I use the follow shortcode shortcodes/bleh.html:

<div>
    {{ .Inner }}
</div>

that way:

{{% bleh %}}
Markdown content
{{% /bleh %}}

it render like this:

<div>
Markdown content
<p></div></p>
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 29, 2015

Member

@bclermont to mee that looks like another issue, but I may be wrong. Looks like a Blackfriday issue, but if that is your complete shortcode, your work around would be to replace % with < and >, because content inside div is ignored by Blackfriday, anyway (or, it should be according to spec).

But could you create another issue for your case?

Member

bep commented Aug 29, 2015

@bclermont to mee that looks like another issue, but I may be wrong. Looks like a Blackfriday issue, but if that is your complete shortcode, your work around would be to replace % with < and >, because content inside div is ignored by Blackfriday, anyway (or, it should be according to spec).

But could you create another issue for your case?

@bclermont

This comment has been minimized.

Show comment
Hide comment
@bclermont

bclermont Aug 29, 2015

Contributor

@bep I think you're right, I will investigate a little more.

is that normal that when I switch {{% to {{< inner markdown don't render anymore?

Contributor

bclermont commented Aug 29, 2015

@bep I think you're right, I will investigate a little more.

is that normal that when I switch {{% to {{< inner markdown don't render anymore?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 29, 2015

Member

@bclermont yes, the < syntax was added in 0.14 to handle use cases with raw HTML that didn't need Markdown rendering (or, the Markdown rendering messed it up badly ...)

Member

bep commented Aug 29, 2015

@bclermont yes, the < syntax was added in 0.14 to handle use cases with raw HTML that didn't need Markdown rendering (or, the Markdown rendering messed it up badly ...)

@bclermont

This comment has been minimized.

Show comment
Hide comment
@bclermont

bclermont Aug 30, 2015

Contributor

I created a separated issue #1387 for that with a project to reproduce the problem

Contributor

bclermont commented Aug 30, 2015

I created a separated issue #1387 for that with a project to reproduce the problem

guy-mograbi-at-gigaspaces added a commit to cloudify-cosmo/docs.getcloudify.org that referenced this issue Sep 13, 2015

fix: gscloak usage was incorrect
ALSO - fix snippets.
It seems hugo has a bug:
gohugoio/hugo#1148
gohugoio/hugo#1387

which is causing YAMLs not to render properly when there's a blank line. removed blank lines.
@PuffinBlue

This comment has been minimized.

Show comment
Hide comment
@PuffinBlue

PuffinBlue Nov 28, 2015

Contributor

The original bug appears to still be active in certain situations as described here:

https://discuss.gohugo.io/t/shortcodes-and-p-tags/2164/5

Should this issue be reopened or a separate one created?

Contributor

PuffinBlue commented Nov 28, 2015

The original bug appears to still be active in certain situations as described here:

https://discuss.gohugo.io/t/shortcodes-and-p-tags/2164/5

Should this issue be reopened or a separate one created?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Nov 29, 2015

Member

Separate.

Member

bep commented Nov 29, 2015

Separate.

tychoish added a commit to tychoish/hugo that referenced this issue Aug 13, 2017

Remove superfluous p-tags around shortcodes
This commit replaces the regexp driven `replaceShortcodeTokens` with a handwritten one.

It wasnt't possible to handle the p-tags case without breaking performance.

This fix actually improves in that area:

```
benchmark                           old ns/op     new ns/op     delta
BenchmarkParsePage                  142738        142667        -0.05%
BenchmarkReplaceShortcodeTokens     665590        575645        -13.51%
BenchmarkShortcodeLexer             176038        181074        +2.86%

benchmark                           old allocs     new allocs     delta
BenchmarkParsePage                  87             87             +0.00%
BenchmarkReplaceShortcodeTokens     9631           9424           -2.15%
BenchmarkShortcodeLexer             274            274            +0.00%

benchmark                           old bytes     new bytes     delta
BenchmarkParsePage                  141830        141830        +0.00%
BenchmarkReplaceShortcodeTokens     52275         35219         -32.63%
BenchmarkShortcodeLexer             30177         30178         +0.00%
```

Fixes #1148

tychoish added a commit to tychoish/hugo that referenced this issue Aug 13, 2017

Use pooled buffer in replaceShortcodes
Even as a copy at the end is needed, this consumes way less memory on Go 1.4.2:

```benchmark                           old ns/op     new ns/op     delta
BenchmarkParsePage                  145979        139964        -4.12%
BenchmarkReplaceShortcodeTokens     633574        631946        -0.26%
BenchmarkShortcodeLexer             195842        187938        -4.04%

benchmark                           old allocs     new allocs     delta
BenchmarkParsePage                  87             87             +0.00%
BenchmarkReplaceShortcodeTokens     9424           9415           -0.10%
BenchmarkShortcodeLexer             274            274            +0.00%

benchmark                           old bytes     new bytes     delta
BenchmarkParsePage                  141830        141830        +0.00%
BenchmarkReplaceShortcodeTokens     35219         25385         -27.92%
BenchmarkShortcodeLexer             30178         30177         -0.00%
```
See #1148
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment