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

chore(gatsby-source-wordpress): Correct glob pattern examples #11804

Conversation

riddla
Copy link
Contributor

@riddla riddla commented Feb 15, 2019

Description

Adds a test for a route blog pattern that is working both on custom hosted WordPress API paths and on API paths from sites that are hosted on wordpress.com.

Related Issues

This is picking up the discussion on the routes documentation within #10871 and the according pull request #10887.

This is picking up the discussion on the routes documentation within gatsbyjs#10871 and the according pull request gatsbyjs#10887.

It adds a test for a pattern that is working both on custom hosted WordPress API paths and on API paths from sites  that are hosted on wordpress.com.
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a lot clearer--will need @pieh or someone with a little more Wordpress experience to make the final call!

@@ -0,0 +1,17 @@
const minimatch = require(`minimatch`)

describe.only(`Glob patterns in the README`, () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe.only(`Glob patterns in the README`, () => {
describe(`Glob patterns in the README`, () => {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also generally, not really sure what these tests are testing. It looks like we're basically testing minimatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As minimatch is used within checkRouteList I just wanted to (programmatically) show that the suggested patterns would work on both types of WordPress installations. Also as a reference point for futures issues/questions regarding the exclude/include patterns. If more examples/bugs come up, we could extend the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thank you for spotting the .only. In other projects I used to have a pre-commit hook for this, spotting .only within tests ... ;)

I corrected it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@riddla cool! But generally - we want to test our own code, otherwise updating our code won't make this test fail (ever).

So we would want to actually write some assertions that validate checkRouteList and import that function, or a function that invokes it, in order to test this more effectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. After all it's just about the examples in the Readme and not the checkRouteList itself. I removed the test and just added the information that it carried to the commit message. Should be sufficient for future reference, if this topic ever comes up again in some issue.

Volker Rose added 2 commits February 15, 2019 22:12
... in favor of giving just an example for future reference:

* Self hosted WordPress API instances are located at `/wp/v2/`, a route example would be `/wp/v2/pages`
* WordPress.com API instances are located at `/wp/v2/sites/example.wordpress.com/`, a route example would be `/pages`
* The pattern `**/pages` matches both types of installations, see http://www.globtester.com/#p=eJzT0tIvSExPLQYACb4ClA%3D%3D&r=eJyzScksU0jOSSwutlXKTSxJzlCy0y8v0C8z0i9ITE8tttEHytvZYFGEJA0AIzoXRg%3D%3D&.
@riddla
Copy link
Contributor Author

riddla commented Feb 19, 2019

@DSchau would you mind having another look?

@DSchau DSchau changed the title fix(gatsby-source-wordpress): Correct glob pattern examples chore(gatsby-source-wordpress): Correct glob pattern examples Feb 19, 2019
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@DSchau DSchau merged commit f6a475b into gatsbyjs:master Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants