search

jacktuck / unfurl

Metadata scraper with support for oEmbed, Twitter Cards and Open Graph Protocol for Node.js :zap:
MIT License
474 stars 51 forks source link

feat: add article type #82

Closed fuji44 closed 3 years ago

fuji44 commented 3 years ago

Please see the issue for the history.

I have a few concerns, so please give me some feedback.

jacktuck commented 3 years ago

Thanks for your contribution.

Do not check for og:type. If you want to be fully compliant with the OGP specification, you have to make it work only for . However, this implementation will always read article:*.

I see. I think this is fine for now and we can revisit this in the future if we need to be stricter.

Is the structure of Metadata good? Added under the root instead of under open_graph to match the structure of the property names.

I see why you did that but think having it under open_graph would be more intuitive. I don't mind having a disparity between the structure and property names.

Is the test code in a good place?

Tests look great , nice work!

I should revisit the testing approach at some point and look at removing nock and the http server.

fuji44 commented 3 years ago

I should revisit the testing approach at some point and look at removing nock and the http server.

I'm not trying to be argumentative here, but can you briefly explain why?

fuji44 commented 3 years ago

Modified article to be part of open_graph.

jacktuck commented 3 years ago

I should revisit the testing approach at some point and look at removing nock and the http server.

I'm not trying to be argumentative here, but can you briefly explain why?

I've not thought about it in any great deal but the current testing approach feels a bit cumbersome.

github-actions[bot] commented 3 years ago

:tada: This PR is included in version 5.6.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: