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

Add plainify template function #1915

Closed
wants to merge 1 commit into from
Closed

Add plainify template function #1915

wants to merge 1 commit into from

Conversation

bep
Copy link
Member

@bep 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 NeedsTriage label Mar 3, 2016
@rdwatters
Copy link
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.

@bep
Copy link
Member Author

bep commented Mar 3, 2016

@rdwatters this is Jsonify:

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

@rdwatters
Copy link
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

@moorereason
Copy link
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?

@bep
Copy link
Member Author

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.

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
Copy link
Member Author

bep commented Mar 12, 2016

PR updated with adjustments.

@moorereason
Copy link
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.

@bep
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Contributor

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

@bep bep deleted the plainify branch April 18, 2017 09:19
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants