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

Add plainify template function #1915

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@bep
Member

bep commented Mar 3, 2016

To strip away any HTML. May be useful for the .Title in head etc.

People may shoot themself in the foot with this, maybe ...

But if someone wants to merge this, I guess it would be fine.

The replacement function is pretty fast.

@bep bep added the NeedsReview label Mar 3, 2016

@rdwatters

This comment has been minimized.

Show comment
Hide comment
@rdwatters

rdwatters Mar 3, 2016

Contributor

@bep This could come in really handy. How many steps away is something like this from a |jsonify? I ask per curiosity and with @digitalcraftsman's #1853 in mind.

Contributor

rdwatters commented Mar 3, 2016

@bep This could come in really handy. How many steps away is something like this from a |jsonify? I ask per curiosity and with @digitalcraftsman's #1853 in mind.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 3, 2016

Member

@rdwatters this is Jsonify:

<script>
{{ . }}
</script>

Member

bep commented Mar 3, 2016

@rdwatters this is Jsonify:

<script>
{{ . }}
</script>

@rdwatters

This comment has been minimized.

Show comment
Hide comment
@rdwatters

rdwatters Mar 4, 2016

Contributor

@bep Working on your suggestion above, but this is getting a little more discourse-appropriate, so I'll comment here instead: https://discuss.gohugo.io/t/possible-to-get-markdown-escaped-as-just-plain-text-if-not-json/2607

Contributor

rdwatters commented Mar 4, 2016

@bep Working on your suggestion above, but this is getting a little more discourse-appropriate, so I'll comment here instead: https://discuss.gohugo.io/t/possible-to-get-markdown-escaped-as-just-plain-text-if-not-json/2607

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 12, 2016

Contributor

Need tests.

Did some research on what others do. Django and Jinja2 call this striptags, which sounds better than plainify to me.

Why do you make it "safe" (return template.HTML)? If we can't guarantee that the resulting string actually is safe, shouldn't we just return a string and let the Go template engine deal with it? What's the harm in return a string?

Contributor

moorereason commented Mar 12, 2016

Need tests.

Did some research on what others do. Django and Jinja2 call this striptags, which sounds better than plainify to me.

Why do you make it "safe" (return template.HTML)? If we can't guarantee that the resulting string actually is safe, shouldn't we just return a string and let the Go template engine deal with it? What's the harm in return a string?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 12, 2016

Member

plainify is in line with the rest of the ify functions. The template.HTML is a mistake in this case, you are correct about that. It doesn't make sense to strip tags then mark it as HTML ... Will add a test or two.

Member

bep commented Mar 12, 2016

plainify is in line with the rest of the ify functions. The template.HTML is a mistake in this case, you are correct about that. It doesn't make sense to strip tags then mark it as HTML ... Will add a test or two.

Add plainify template function
To strip away any HTML. May be useful for the .Title in head etc.

People may shoot themself in the foot with this, maybe ...

But if someone wants to merge this, I guess it would be fine.

The replacement function is pretty fast.
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 12, 2016

Member

PR updated with adjustments.

Member

bep commented Mar 12, 2016

PR updated with adjustments.

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 12, 2016

Contributor

My point about the name was that "plain" isn't very descriptive or intuitive to me. Our existing -ify funcs (where I use renderer very loosely):

  • emojify sends the content through an emoji renderer.
  • markdownify sends the content through a markdown renderer.
  • jsonify sends the content through a JSON renderer.

So, plainify sends the content through a plain renderer? What does that mean? Does it convert from en_GB to en_US? 😄 Most people wouldn't know with certainty without looking it up in the docs that we mean it's a plain text renderer.

More intuitive options: textify or stripHTML.

Contributor

moorereason commented Mar 12, 2016

My point about the name was that "plain" isn't very descriptive or intuitive to me. Our existing -ify funcs (where I use renderer very loosely):

  • emojify sends the content through an emoji renderer.
  • markdownify sends the content through a markdown renderer.
  • jsonify sends the content through a JSON renderer.

So, plainify sends the content through a plain renderer? What does that mean? Does it convert from en_GB to en_US? 😄 Most people wouldn't know with certainty without looking it up in the docs that we mean it's a plain text renderer.

More intuitive options: textify or stripHTML.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 12, 2016

Member

We already have Plain and PlainWords -- this is the template func version of that. If people have problem understanding what it does, we should do a better job documenting it.

Member

bep commented Mar 12, 2016

We already have Plain and PlainWords -- this is the template func version of that. If people have problem understanding what it does, we should do a better job documenting it.

@spf13

This comment has been minimized.

Show comment
Hide comment
@spf13

spf13 Mar 21, 2016

Contributor

I reviewed the code and it's good.

As for the name, at first I was inclined to agree with @bep's validation for the name as it's consistent with other things we have. In using the function it felt wrong and the name did confuse me.

I think it should be called either stripTags or stripHTML.

I have a few reasons for this:

  • Plain & PlainWords makes sense as a field/noun on the content. You are accessing the plain version of the content.
  • I don't think plain makes sense as a verb/function/action.
  • the other "ify" functions do render things, this one is a filter.

The function being called inside of this (and inside Plain) is helpers.StripHTML which feels like the right descriptive name for what it is doing.

In retrospect, the helpers.StripHTML should have been named StripTags, but as it was only being used for HTML the name is accurate enough.

Contributor

spf13 commented Mar 21, 2016

I reviewed the code and it's good.

As for the name, at first I was inclined to agree with @bep's validation for the name as it's consistent with other things we have. In using the function it felt wrong and the name did confuse me.

I think it should be called either stripTags or stripHTML.

I have a few reasons for this:

  • Plain & PlainWords makes sense as a field/noun on the content. You are accessing the plain version of the content.
  • I don't think plain makes sense as a verb/function/action.
  • the other "ify" functions do render things, this one is a filter.

The function being called inside of this (and inside Plain) is helpers.StripHTML which feels like the right descriptive name for what it is doing.

In retrospect, the helpers.StripHTML should have been named StripTags, but as it was only being used for HTML the name is accurate enough.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Mar 21, 2016

Member

Well, I don't agree - the other ify functions are also transformers, they transform a piece of content from a state/format to another. So does this. The interface of plainify is that it will have the same effect on the input as the Plain and PlainWords. This involves currently stripping some HTML tags and converting some newlines.

Feel free to add another template function with a more specific use case, but this isn't that function.

Maybe we should have a stripHTML function and delete this one. Up to you. I have no strong feelings about that.

The only thing I agree about here is that it it reads bad as a verb. But that is also true for jsonify, emojify etc. i.e. all of them. But the value of the naming convention makes up for the stupidity.

Member

bep commented Mar 21, 2016

Well, I don't agree - the other ify functions are also transformers, they transform a piece of content from a state/format to another. So does this. The interface of plainify is that it will have the same effect on the input as the Plain and PlainWords. This involves currently stripping some HTML tags and converting some newlines.

Feel free to add another template function with a more specific use case, but this isn't that function.

Maybe we should have a stripHTML function and delete this one. Up to you. I have no strong feelings about that.

The only thing I agree about here is that it it reads bad as a verb. But that is also true for jsonify, emojify etc. i.e. all of them. But the value of the naming convention makes up for the stupidity.

@spf13

This comment has been minimized.

Show comment
Hide comment
@spf13

spf13 Mar 22, 2016

Contributor

Thank you for the clarification, I didn't realize that this function was doing more than just stripping.

LGTM. merged as e5e1bcc

Contributor

spf13 commented Mar 22, 2016

Thank you for the clarification, I didn't realize that this function was doing more than just stripping.

LGTM. merged as e5e1bcc

@spf13 spf13 closed this Mar 22, 2016

@moorereason

This comment has been minimized.

Show comment
Hide comment
@moorereason

moorereason Mar 22, 2016

Contributor

Just to be clear, this new plainify function simply calls helpers.StripHTML on the input. It does nothing else.

Contributor

moorereason commented Mar 22, 2016

Just to be clear, this new plainify function simply calls helpers.StripHTML on the input. It does nothing else.

@bep bep deleted the bep:plainify branch Apr 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment