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

Add docs schema #2037

Closed
wants to merge 7 commits into from
Closed

Add docs schema #2037

wants to merge 7 commits into from

Conversation

dingo-d
Copy link
Member

@dingo-d dingo-d commented Apr 3, 2022

This PR addresses the #1722 issue.

I've made some schema examples with automated tools (PhpStorm and XMLSpy), then went ahead and abstracted things according to default phpcs.xsd schema.

Once added every documentation type should define the

xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"

and

xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/WordPress/WordPress-Coding-Standards/develop/WordPress/Docs/docs-schema.xsd"

tags in the main documentation tag of the documentation xml files.

Like:

<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/WordPress/WordPress-Coding-Standards/develop/WordPress/Docs/docs-schema.xsd title="Array Indentation">

I've added the xsd file in the Docs folder, kinda felt natural, but we can move it to the root of the project.

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 Yeah! Nice!

Important side-note: I do not think we should merge this to WPCS.
In my opinion, this should be pulled to PHPCS upstream and if it wouldn't be accepted there (and/or in the mean time while the upstream PR is waiting for a response), to PHPCSUtils.

Pulling it to either of the above makes it available to a wider group of external standards, not just to WPCS (including to PHPCSExtra which will be a dependency of WPCS as of v 3.0.0).

Using this PR to fine-tune the details before pulling this to PHPCS itself, is, of course, perfectly fine.


Now, as for the review:

  • Should the text string elements (standard, code) have a whitespace restriction ?
    To allow for consistent display on the command-line, indentation should be using spaces.
    Do we want to enforce this ?
    https://www.w3schools.com/xml/schema_facets.asp
  • Looks like tabs are used in the XSD file. Should this be spaces ?
  • Should <xs:any minOccurs="0"/> be added to documentation ?
    This would allow for optionally adding additional elements not officially supported by PHPCS.
    For example: we could choose to add a <url> element at the end of a doc with a link to the official docs on the Make website.
    Without the <xs:any minOccurs="0"/>, this would not be allowed and the doc would no longer validate.
  • Along the same lines, should <xs:anyAttribute/> be allowed on <documentation> and <standard> elements ?
    Use-case example: for PHPCompatibility, I can imagine, we'd could add a php="5.6" attribute to each <standard> element to indicate what PHP version the standard applies to.

Final question: have you tested the existing documentation against the schema to see if it would validate ?

Follow-up actions once the schema is available:

  • Add the schema reference to all currently existing XML sniff docs.
  • Add validation of the docs against the schema to the basic QA workflow.
  • See about adding basic XML CS for the docs is possible.
    (The difficulty with that will be that it needs to do a diff which means that we need to find a solution against having to hard-code the name of each individual doc in the workflow.)

WordPress/Docs/docs-schema.xsd Outdated Show resolved Hide resolved
WordPress/Docs/docs-schema.xsd Outdated Show resolved Hide resolved
WordPress/Docs/docs-schema.xsd Outdated Show resolved Hide resolved
@dingo-d
Copy link
Member Author

dingo-d commented Apr 4, 2022

Thanks for the in-depth review, will fix the comments probably today after work.

Should the text string elements (standard, code) have a whitespace restriction?
To allow for consistent display on the command-line, indentation should be using spaces.
Do we want to enforce this?

This is a tricky one. Because we would then need to decide what is the minimum resolution we would want to allow.
Again, here we can have retina displays, regular laptop displays, people using smaller font sizes, etc.

But even then, the screen size itself doesn't define what is the size of their terminal (I usually split my terminal windows in two side by side).

How do we define the minimal number of characters on a line?

Looks like tabs are used in the XSD file. Should this be spaces?

I am not 100% what we enforce. I always use tabs, because they're more accessible, so that could have been my IDE. Happy to turn them into spaces 🙂

Should <xs:any minOccurs="0"/> be added to documentation ?
This would allow for optionally adding additional elements not officially supported by PHPCS.
For example: we could choose to add a element at the end of a doc with a link to the official docs on the Make website.
Without the <xs:any minOccurs="0"/>, this would not be allowed and the doc would no longer validate.

I'm for it. For php parts those could be links to PSR implementations and similar 👍🏼

Along the same lines, should xs:anyAttribute/ be allowed on and elements ?
Use-case example: for PHPCompatibility, I can imagine, we'd could add a php="5.6" attribute to each element to > indicate what PHP version the standard applies to.

I like this idea, definitely for it.

@jrfnl
Copy link
Member

jrfnl commented Apr 4, 2022

This is a tricky one. Because we would then need to decide what is the minimum resolution we would want to allow.
Again, here we can have retina displays, regular laptop displays, people using smaller font sizes, etc.

But even then, the screen size itself doesn't define what is the size of their terminal (I usually split my terminal windows in two side by side).

For the docs display on the CLI, it's not about screen size or anything, it's more about the hard-coded limits defined in the generator classes in PHPCS itself...

How do we define the minimal number of characters on a line?

Not sure about a minimum, but a maximum should be definable based on PHPCS.

I am not 100% what we enforce. I always use tabs, because they're more accessible, so that could have been my IDE. Happy to turn them into spaces

For XML docs WPCS enforces tabs, but everywhere else enforces spaces, including PHPCS and PHPCSUtils, so with a mind to pulling it upstream, it should be spaces.

@dingo-d
Copy link
Member Author

dingo-d commented Apr 5, 2022

For the docs display on the CLI, it's not about screen size or anything, it's more about the hard-coded limits defined in the generator classes in PHPCS itself...

Wasn't aware of that. From what I can see the default is 100 (hardcoded) so we can set that as a default length (https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Generators/Text.php#L248). Doesn't look like it can be customized tho 🤷🏼‍♂️

@jrfnl
Copy link
Member

jrfnl commented Apr 5, 2022

For the docs display on the CLI, it's not about screen size or anything, it's more about the hard-coded limits defined in the generator classes in PHPCS itself...

Wasn't aware of that. From what I can see the default is 100 (hardcoded) so we can set that as a default length (https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Generators/Text.php#L248). Doesn't look like it can be customized tho 🤷🏼‍♂️

Correct - though that only applies to the title at the top (with the prefix and the | ... | markers around it and the code samples.

The contents of standard blocks and the title of the code samples will both wrap.

The code in the code samples will not, but as they are placed next to each other and have those | markers around them as well, individual code lines need to be max 48 char, but that can't be enforced via an XSD.

@dingo-d
Copy link
Member Author

dingo-d commented Apr 7, 2022

I tried adding <xs:any minOccurs="0"/> inside the documentation to allow for any element addition (the <url> that you mentioned) like this:

<xs:element name="documentation">
        <xs:complexType>
            <xs:sequence maxOccurs="unbounded">
                <xs:element name="standard" type="xs:string"/>
                <xs:element name="code_comparison" type="code_comparisonType"/>
                <xs:any minOccurs="0"/>
            </xs:sequence>
            <xs:attribute name="title" use="required" type="xs:string"/>
            <xs:anyAttribute/>
        </xs:complexType>
    </xs:element>

But the XML validator throws an error

Cos-nonambig: Standard And WC[##any] (or Elements From Their Substitution Group) Violate "Unique Particle Attribution". During Validation Against This Schema, Ambiguity Would Be Created For Those Two Particles.

My guess is that the unique particle attribution comes from the fact that the validator cannot know if the element is duplicated or not if it's set to any 🤷🏼‍♂️

I've tried adding restrictions to the standard element (to limit whitespace to spaces), but I keep getting errors. I tried adding a standardType but got error

docs-schema.xsd
<?xml version="1.0" encoding="UTF-8"?>
<xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema" elementFormDefault="qualified">
    <xs:element name="documentation">
        <xs:complexType>
            <xs:sequence maxOccurs="unbounded">
                <xs:element name="standard" type="standardType"/>
                <xs:element name="code_comparison" type="code_comparisonType"/>
                <xs:any minOccurs="0"/>
            </xs:sequence>
            <xs:attribute name="title" use="required" type="xs:string"/>
            <xs:anyAttribute/>
        </xs:complexType>
    </xs:element>

    <xs:complexType name="standardType">
        <xs:simpleContent>
            <xs:restriction base="xs:string">
                <xs:whiteSpace value="replace"/>
            </xs:restriction>
        </xs:simpleContent>
    </xs:complexType>

    <xs:complexType name="code_comparisonType">
        <xs:sequence>
            <xs:element name="code" type="codeType" maxOccurs="2" minOccurs="2"/>
        </xs:sequence>
    </xs:complexType>

    <xs:complexType name="codeType">
        <xs:simpleContent>
            <xs:extension base="xs:string">
                <xs:attribute name="title" use="required" type="xs:string"/>
            </xs:extension>
        </xs:simpleContent>
    </xs:complexType>
</xs:schema>

Src-ct.2.1: Complex Type Definition Representation Error For Type 'standardType'. When Is Used, The Base Type Must Be A ComplexType Whose Content Type Is Simple, Or, Only If Restriction Is Specified, A Complex Type With Mixed Content And Emptiable Particle, Or, Only If Extension Is Specified, A Simple Type. 'string' Satisfies None Of These Conditions.

Add additional validation types for title length and standard attributes.
@dingo-d
Copy link
Member Author

dingo-d commented Apr 22, 2022

I've pushed the latest changes, and it validates against xmllint. I've also validated the existing xml documentation with

find ./WordPress/Docs -type f -name "*.xml" -print0 | while read -d $'\0' file; do xmllint --noout --schema ./WordPress/Docs/docs-schema.xsd $file; done

And it all checked out.

For testing it out I'll try to add false examples to see if the validation would fail.

This will enable the addition of any attribute on the documentation element, like PHP version for PHPCompatibility or similar.
@dingo-d
Copy link
Member Author

dingo-d commented May 21, 2022

The PR was moved to PHPCSDevTools repo. Once merged this PR can be closed.

@jrfnl
Copy link
Member

jrfnl commented Jun 14, 2022

The upstream PR has been merged and will be included in the PHPCSDevTools 2.0.0 release.

I'm closing this PR now. Once PHPCSDevTools 2.0.0 has been released (expected in the next few weeks), a PR will be submitted to this repo to update the dependency and to add validation against the schema for the docs contained in this repo.

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