otiai10 / opengraph

Open Graph Parser for Go
https://pkg.go.dev/github.com/otiai10/opengraph
MIT License
65 stars 13 forks source link

Video tag does not seems to be parsed #20

Closed AyWa closed 3 years ago

AyWa commented 3 years ago

Video tag does not seems to be parsed. I will try to implement a base version of it with test

otiai10 commented 3 years ago

@AyWa Can you give me some example pages including video tags?

AyWa commented 3 years ago

For sure, you can check any video on youtube, or any other website that might be display as video on twitter / FB etc.

https://www.youtube.com/watch?v=1MxA0i2rxQo

Screen Shot 2020-12-11 at 2 00 01 PM

I plan to do simple implementation for it today :)

otiai10 commented 3 years ago

Thx AyWa! 1 thing to add, I'm planning to release v2 in few weeks, can you send your PR to develop? and any feedback on develop will be welcomed ;)

AyWa commented 3 years ago

Sure let me do that on develop branch :) Should we include https://github.com/otiai10/opengraph/commit/7d11cc903f52dc66ec702cb21e45fd7b3d84ecdb to develop branch too ?

otiai10 commented 3 years ago

I already rebased develop branch and Walk is there ;) Can you please just branch out from the latest develop 🙏

AyWa commented 3 years ago

So I am testing develop branch, and it seems better than before 👍

Just wonder if this part:

https://github.com/otiai10/opengraph/blob/develop/opengraph.go#L127

can be inside the opengraph.New() ? because now I need to do something like:

        og := opengraph.New(reqURL)
        og.Intent.TrustedTags = []string{opengraph.HTMLMetaTag, opengraph.HTMLTitleTag, opengraph.HTMLLinkTag}
                og.Walk(....
otiai10 commented 3 years ago

Thank you, good catch!

The only guy who needs to know what TrustedTags are is walk, therefore I'd prefer below:

func (og *OpenGraph) walk(node *html.Node) error {

    if len(og.Intent.TrustedTags) == 0 {
        if og.Intent.Strict {
            og.Intent.TrustedTags = []string{HTMLMetaTag}
        } else {
            og.Intent.TrustedTags = []string{HTMLMetaTag, HTMLTitleTag, HTMLLinkTag}
        }
    }

    // following stuff here
}

Let me know your opinion.

otiai10 commented 3 years ago

Wait, if it's like that, that code block will be executed for all walk which are called recursively.

Then I'd like to suggest:

func (og *OpenGraph) Parse(body io.Reader) error {
    // other stuff here
-   og.walk(node)
+   og.Walk(node)
    return nil
}

func (og *OpenGraph) Walk(n *html.Node) error {
+   if len(og.Intent.TrustedTags) == 0 {
+       if og.Intent.Strict {
+           og.Intent.TrustedTags = []string{HTMLMetaTag}
+       } else {
+           og.Intent.TrustedTags = []string{HTMLMetaTag, HTMLTitleTag, HTMLLinkTag}
+       }
+   }
    return og.walk(n)
}
AyWa commented 3 years ago

I think it is fine for me too 👍

otiai10 commented 3 years ago

Closed by #21