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

PHPCS template: fix XML error in ruleset #171

Merged
merged 1 commit into from Aug 14, 2018

Conversation

@jrfnl
Contributor

jrfnl commented Aug 12, 2018

As reported by @dimadin on Slack, when using the current .phpcs.xml.dist ruleset, a
phpcs: Ruleset <path>/.phpcs.xml.dist is not valid
error is thrown.

This was caused by an unclosed comment in the XML. This PR fixes it.

The PR can be tested by:

  1. Save the current ruleset locally and try to use it to see the error.
  2. Save the ruleset from this PR and try again. The ruleset error will be gone.
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Aug 12, 2018

Contributor

Side-note: to prevent this issue in the future, I'd be happy to add XML validation to the Travis script for all XML files ?

Contributor

jrfnl commented Aug 12, 2018

Side-note: to prevent this issue in the future, I'd be happy to add XML validation to the Travis script for all XML files ?

@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 13, 2018

Contributor

@jrfnl That would be interesting. What did you have in mind for this? The only thing that comes to mind is xmllint.

Contributor

swissspidy commented Aug 13, 2018

@jrfnl That would be interesting. What did you have in mind for this? The only thing that comes to mind is xmllint.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Aug 13, 2018

Contributor

@jrfnl That would be interesting. What did you have in mind for this? The only thing that comes to mind is xmllint.

@swissspidy That's how I've implemented it a while ago in WPCS - both validation as well as an XML codestyle check. See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/eccb1f721f95fdcf5c2e383340aa4b094c0317ad/.travis.yml#L112-L122

Contributor

jrfnl commented Aug 13, 2018

@jrfnl That would be interesting. What did you have in mind for this? The only thing that comes to mind is xmllint.

@swissspidy That's how I've implemented it a while ago in WPCS - both validation as well as an XML codestyle check. See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/eccb1f721f95fdcf5c2e383340aa4b094c0317ad/.travis.yml#L112-L122

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 14, 2018

Member

@jrfnl Linting the XML sounds good. However, this is currently a template file, so it might not pass the linter because of that. What I had been thinking recently is to let Travis actually use the scaffolding command to scaffold a plugin/theme, and then run tests against that scaffolded result.

I'll open a separate issue to that regard.

Thanks for this PR, @jrfnl !

Member

schlessera commented Aug 14, 2018

@jrfnl Linting the XML sounds good. However, this is currently a template file, so it might not pass the linter because of that. What I had been thinking recently is to let Travis actually use the scaffolding command to scaffold a plugin/theme, and then run tests against that scaffolded result.

I'll open a separate issue to that regard.

Thanks for this PR, @jrfnl !

@schlessera schlessera merged commit 2ee679f into wp-cli:master Aug 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jrfnl jrfnl deleted the jrfnl:feature/fix-phpcs-ruleset-template branch Aug 14, 2018

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Aug 14, 2018

Contributor

However, this is currently a template file, so it might not pass the linter because of that.

It actually does pass the linter as it is now this PR has been merged, so that's not an issue.

What I had been thinking recently is to let Travis actually use the scaffolding command to scaffold a plugin/theme, and then run tests against that scaffolded result.

That sounds interesting ;-)

I haven't used the scaffold command myself yet, so this may already be covered, but in case it's not: it may also be useful to adjust the values for some of the customizable properties in the XML ruleset when doing the scaffolding.

Contributor

jrfnl commented Aug 14, 2018

However, this is currently a template file, so it might not pass the linter because of that.

It actually does pass the linter as it is now this PR has been merged, so that's not an issue.

What I had been thinking recently is to let Travis actually use the scaffolding command to scaffold a plugin/theme, and then run tests against that scaffolded result.

That sounds interesting ;-)

I haven't used the scaffold command myself yet, so this may already be covered, but in case it's not: it may also be useful to adjust the values for some of the customizable properties in the XML ruleset when doing the scaffolding.

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