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

XSD docs schema #80

Merged
merged 51 commits into from
Jun 14, 2022
Merged

XSD docs schema #80

merged 51 commits into from
Jun 14, 2022

Conversation

dingo-d
Copy link
Collaborator

@dingo-d dingo-d commented May 21, 2022

This PR adds the docs schema defined in WordPress Coding Standards.

The initial test setup and folder structure were done by @jrfnl.

I added the test cases, and also found one small issue in the schema (prooving why tests are necessary and awesome 😄).

@dingo-d
Copy link
Collaborator Author

dingo-d commented May 21, 2022

@jrfnl Can we silence the Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned sniff?

Without array alignment:

image

With the array alignment:

image

The second one just looks... odd :S

Also, I'm always for loosening the 120 line length character limit (I'm getting the phpcs errors for 121 or 123 line lenght 😅).

@jrfnl
Copy link
Member

jrfnl commented May 21, 2022

@jrfnl Can we silence the Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned sniff?

Also, I'm always for loosening the 120 line length character limit (I'm getting the phpcs errors for 121 or 123 line lenght 😅).

Yup, Already have a commit lined up to silence the line length sniff for the test directory.

The array double arrow alignment sniff will be replaced by a better sniff in due course. For the time being, feel free to silence it in the troublesome file by using the following in the class docblock (I'm doing the same elsewhere):

 * @phpcs:disable Squiz.Arrays.ArrayDeclaration.DoubleArrowNotAligned -- If needed, fix once replaced by better sniff.

@jrfnl
Copy link
Member

jrfnl commented May 21, 2022

@dingo-d Thanks for getting this set up! I'll have a look at this a little later. Will probably ask you to rebase after I've pulled (and merged) the other test PRs, which contain the full test setup.

@jrfnl jrfnl added this to the 2.0.0 milestone May 21, 2022
@jrfnl
Copy link
Member

jrfnl commented May 21, 2022

Oh, one point of feedback already: would be good to add this new feature to the README with some code samples of how to:

  1. Refer to the XSD in doc XML files.
  2. How to validate doc XML files against the XSD

@jrfnl
Copy link
Member

jrfnl commented May 21, 2022

Oh and another thing before I forget again: let's add the XSD file to the website files array - that way the XSD file can be referred to using a canonical URL.

@jrfnl jrfnl force-pushed the xsd-docs-schema branch from dcabab4 to 45847b5 Compare May 29, 2022 06:26
dingo-d and others added 10 commits May 30, 2022 13:11
Add the test for the docs schema
Added a few test cases, modify the assertions to accomodate the output of the command - check for valid message and part of the error message in case of the failure.
Added lax contents processing for any attribute in the standards type. Without it, the attributes must be explicitly defined in the xsd, and we don't know which attribute(s) some standard may want to add to their documentation.
* Install `xmllint` in the test workflows to allow PHPUnit to run it.
@jrfnl jrfnl force-pushed the xsd-docs-schema branch from 45847b5 to a2e7933 Compare May 30, 2022 11:12
dingo-d added 6 commits June 4, 2022 13:29
Add the test for the docs schema
Partal fix. Some things will still fail for the current rules (array items alignment and line lenght in some cases).
Add the instructions on how to use the xsd file in the sniff documentation file.
@dingo-d
Copy link
Collaborator Author

dingo-d commented Jun 4, 2022

@jrfnl I've added the changes you've suggested. Can you check the PR to see if I missed anything? If all is ok, you can just squash and merge the PR 🙂

@dingo-d
Copy link
Collaborator Author

dingo-d commented Jun 11, 2022

I suspect not, but can CDATA be enforced for the contents of the <standard> and <code> elements ?

From what I've googled, it's not possible to enforce CDATA. They already have to be a string type by using the <xs:extension base="xs:string"> element in the xsd file.

I've addressed I think most of the issues you've mentioned. One thing I'm not sure I understood was the debugging of the message with var_export. I tried adding your suggestion but didn't get anything. And when I add var_export without the second argument true I get the output on all the test runs, which I doubt we want.

I still ned to add additional test cases you've mentioned in the comment.

I've created a small bash script for myself that looks like this

#!/bin/bash

# PHPCS
for file in /home/dzoljom/Projects/Personal/phpcs/vendor/squizlabs/php_codesniffer/src/Standards/*/Docs/*/*Standard.xml; 
do
	xmllint --noout --schema /home/dzoljom/Projects/Personal/PHPCSDevTools/DocsXsd/phpcsdocs.xsd $file
done

# PHPCSExtra
for file in /home/dzoljom/Projects/Personal/PHPCSExtra/*/Docs/*/*Standard.xml; 
do
	xmllint --noout --schema /home/dzoljom/Projects/Personal/PHPCSDevTools/DocsXsd/phpcsdocs.xsd $file
done

# PHPCSDevTools
for file in /home/dzoljom/Projects/Personal/PHPCSDevTools/*/Docs/*/*Standard.xml; 
do
	xmllint --noout --schema /home/dzoljom/Projects/Personal/PHPCSDevTools/DocsXsd/phpcsdocs.xsd $file
done

# WPCS
for file in /home/dzoljom/Projects/Personal/WordPress-Coding-Standards/*/Docs/*/*Standard.xml; 
do
	xmllint --noout --schema /home/dzoljom/Projects/Personal/PHPCSDevTools/DocsXsd/phpcsdocs.xsd $file
done

(the path is obviously specific for my use-case but that can be changed easily)

I've gotten the following failures, only in phpcs itself:

.../php_codesniffer/src/Standards/Generic/Docs/Classes/OpeningBraceSameLineStandard.xml:21: element code: Schemas validity error : Element 'code': This element is not expected.
.../php_codesniffer/src/Standards/Generic/Docs/Classes/OpeningBraceSameLineStandard.xml fails to validate
.../php_codesniffer/src/Standards/Generic/Docs/Formatting/SpaceAfterNotStandard.xml:18: element code: Schemas validity error : Element 'code': This element is not expected.
.../php_codesniffer/src/Standards/Generic/Docs/Formatting/SpaceAfterNotStandard.xml fails to validate
.../php_codesniffer/src/Standards/Generic/Docs/WhiteSpace/ArbitraryParenthesesSpacingStandard.xml:18: element code: Schemas validity error : Element 'code': This element is not expected.
.../php_codesniffer/src/Standards/Generic/Docs/WhiteSpace/ArbitraryParenthesesSpacingStandard.xml fails to validate

All of those have more than two code elements inside a comparison. I would say that's invalid and should be fixed in the documentation. All the rest were fine 🙂

Other issues

Should the "prefix" be added to the fixture files ? Or maybe let some have it and some not ?

I can add it, but if I add it to one, I'd add it to all, and to the examples in the readme.

Should the xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" and xsi:noNamespaceSchemaLocation="/path/to/phpcsdocs.xsd" bits be added to the elements in the fixtures ?

Maybe after this PR is merged, and the phpcsdocs.xsd is deployed to te webiste? That way I can add the correct URL to the location of the schema.

Should the above also be added to the Docs which already exist in this repo ? (only one in PHPCSDebug).

I'd also do this in the PR after this one, once we have the xsd file live on the URL.

Edit

The phpcs checks are failing, I think on the XML validation for the fixtures that have intentional parse errors, should we silence those somehow, or exclude them from parsing?

dingo-d added 3 commits June 11, 2022 11:02
Fix issues not covered by the new tests.
Add more cases.
@dingo-d dingo-d requested a review from jrfnl June 11, 2022 09:05
@jrfnl
Copy link
Member

jrfnl commented Jun 11, 2022

All of those have more than two code elements inside a comparison. I would say that's invalid and should be fixed in the documentation. All the rest were fine 🙂

🎉 That means it works and confirms the need for the XSD! I've lined up a PR to send to PHPCS itself to fix those (looking back, I probably introduced those errors... 🙈 ) and I will suggest PHPCS start validating docs in CI too.
I will pull the PR once DevTools 2.0 has been released.

Should the "prefix" be added to the fixture files ? Or maybe let some have it and some not ?

I can add it, but if I add it to one, I'd add it to all, and to the examples in the readme.

Sounds like a good idea.

Should the xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" and xsi:noNamespaceSchemaLocation="/path/to/phpcsdocs.xsd" bits be added to the elements in the fixtures ?

Maybe after this PR is merged, and the phpcsdocs.xsd is deployed to te webiste? That way I can add the correct URL to the location of the schema.

Should the above also be added to the Docs which already exist in this repo ? (only one in PHPCSDebug).

I'd also do this in the PR after this one, once we have the xsd file live on the URL.

Why can't it be added already even though the location is (not yet) valid ? The website deploy - including the XSD file - will happen on the 2.0 release, so otherwise those changes would have to wait until a 2.0.1 release.

The phpcs checks are failing, I think on the XML validation for the fixtures that have intentional parse errors, should we silence those somehow, or exclude them from parsing?

It's actually failing on two PHPCS errors on the test file (unnecessary use of double quotes). See the code view ;-)

@jrfnl
Copy link
Member

jrfnl commented Jun 11, 2022

One thing I'm not sure I understood was the debugging of the message with var_export. I tried adding your suggestion but didn't get anything. And when I add var_export without the second argument true I get the output on all the test runs, which I doubt we want.

The output of var_export would (should) only be shown on failing tests, so as part of the $message parameter. Without a failing test, it would be correct that it doesn't show.

Or is that not what you meant ? How can I help explain this more clearly ?

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Just some very very minor final things ;-) (aside from the replies I posted Saturday).

.github/workflows/cs.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Tests/DocsXsd/DocsXsdTest.php Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dingo-d
Copy link
Collaborator Author

dingo-d commented Jun 13, 2022

@jrfnl Fixed all the outstanding issues. I've added a reference to the schema like this:

<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd" title="Some sniff docs">

Is this correct (URL)?

@dingo-d dingo-d requested a review from jrfnl June 13, 2022 16:30
@jrfnl
Copy link
Member

jrfnl commented Jun 14, 2022

@jrfnl Fixed all the outstanding issues. I've added a reference to the schema like this:

🙌🏻

Is this correct (URL)?

Is is based on how the file has been added to the website deploy setup, so yeah 👍🏻

... to a working minimal example
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

@dingo-d You sir, are an absolute hero! Well done!

@jrfnl jrfnl merged commit d6641f6 into develop Jun 14, 2022
@jrfnl jrfnl deleted the xsd-docs-schema branch June 14, 2022 01:53
jrfnl added a commit that referenced this pull request Sep 23, 2022
* Introduces an XSD schema file to allow for validating PHPCS XML docs files, i.e. the `Standard/Docs/Category/*Standard.xml` files.
* Adds this file to the website build script so the file will also be available at a canonical URL via the website.
* Adds validation of the XSD file against the schema for XSD to the CI/GH Actions workflows.
* Adds validation of the PHPCSDebug XML docs against the new XSD schema file to the CI/GH Actions workflows.
* Adds documentation about the XSD schema and how to use it to the README.
* Adds extensive tests for the XSD schema to the integration test suite.
* Adds a reference to the schema to the existing XML doc for the PHPCSDebug standard.

Notes:
* The XSD allows for multiple sets of `<standard>` elements, with each optionally having one or more `<code_comparison>`s.
* The XSD allows for additional arbitrary attributes on select elements in the XML files.
    These use `lax` contents processing, as without it, the attributes must be explicitly defined in the XSD, and we don't know which attribute(s) some standard may want to add to their documentation.
* The XSD has also been real-world tested against XML documentation files from PHPCS itself, WordPressCS, PHPCSExtra and more. Those all pass, except for a few were the XML doc actually contained errors, which confirms that the XSD will work in real-life situations.

Co-authored-by: Juliette <[email protected]>
jrfnl added a commit that referenced this pull request Sep 23, 2022
* Introduces an XSD schema file to allow for validating PHPCS XML docs files, i.e. the `Standard/Docs/Category/*Standard.xml` files.
* Adds this file to the website build script so the file will also be available at a canonical URL via the website.
* Adds validation of the XSD file against the schema for XSD to the CI/GH Actions workflows.
* Adds validation of the PHPCSDebug XML docs against the new XSD schema file to the CI/GH Actions workflows.
* Adds documentation about the XSD schema and how to use it to the README.
* Adds extensive tests for the XSD schema to the integration test suite.
* Adds a reference to the schema to the existing XML doc for the PHPCSDebug standard.

Notes:
* The XSD allows for multiple sets of `<standard>` elements, with each optionally having one or more `<code_comparison>`s.
* The XSD allows for additional arbitrary attributes on select elements in the XML files.
    These use `lax` contents processing, as without it, the attributes must be explicitly defined in the XSD, and we don't know which attribute(s) some standard may want to add to their documentation.
* The XSD has also been real-world tested against XML documentation files from PHPCS itself, WordPressCS, PHPCSExtra and more. Those all pass, except for a few were the XML doc actually contained errors, which confirms that the XSD will work in real-life situations.

Co-authored-by: Juliette <[email protected]>
@jrfnl jrfnl mentioned this pull request Sep 23, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants