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

feat: add article type #82

Merged
merged 2 commits into from
Aug 1, 2021
Merged

Conversation

fuji44
Copy link
Contributor

@fuji44 fuji44 commented Jul 27, 2021

Please see the issue for the history.

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

  • 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 <meta property="og:type" content="article" />. However, this implementation will always read article:*.
  • Is the structure of Metadata good?
    • Added under the root instead of under open_graph to match the structure of the property names.
  • Is the test code in a good place?

@jacktuck
Copy link
Owner

jacktuck commented Jul 27, 2021

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

fuji44 commented Jul 27, 2021

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

fuji44 commented Jul 28, 2021

Modified article to be part of open_graph.

@jacktuck
Copy link
Owner

jacktuck commented Aug 1, 2021

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.

@jacktuck jacktuck merged commit 8945de7 into jacktuck:master Aug 1, 2021
@github-actions
Copy link

github-actions bot commented Aug 1, 2021

🎉 This PR is included in version 5.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@fuji44 fuji44 deleted the feature/add-article-type branch August 2, 2021 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants