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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Charset field to the OpenGraph struct #3

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@cometkim
Contributor

cometkim commented Apr 13, 2018

Again! 馃憢 @dyatlov

Ref: mattermost/mattermost-server#8341

I want an ability to force the opengraph information to be encoded as utf-8. so I just added to charset field for it.

Please check my PR, or let me know if you have any better suggestion.

Thank you :)

@dyatlov

This comment has been minimized.

Owner

dyatlov commented Apr 13, 2018

Charset is not part of OpenGraph standard, so I would rather not add it here, it can be parsed separately.
Also, I don't think that you're parsing content-type meta correctly: https://www.w3.org/Protocols/rfc1341/4_Content-Type.html
Encoding definition can be a quoted string.

@dyatlov

This comment has been minimized.

Owner

dyatlov commented Apr 13, 2018

I think https://github.com/dyatlov/go-htmlinfo is a better place for adding Charset into.
It includes opengraph and other types of info.

@cometkim

This comment has been minimized.

Contributor

cometkim commented Apr 13, 2018

Ok, Thanks for your quick review and nice suggestion.

And you got me taking a look for the standard! 馃檱 I'll try to make a new PR to there soon. Thank you again!

@cometkim cometkim closed this Apr 13, 2018

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