DocbookCS: remove SimparaSniff

[vrana]

I don't see any purpose for this rule. This argument makes no sense to me:

You can't have block elements in a simpara tags. Which helps prevent people writing dumb stuff inside para tags just because they think they might need it.

If people want to write dumb stuff inside para tags then they would just use them.

Moreover, simpara renders differently (the first uses simpara, second para):

Changing all para to simpara to satisfy this rule would completely break how the docs is rendered and what was the intent of the author.

[jordikroon]

This rule reflects the current style guide that has been in place for a while now.

So before we remove this, we have to re-evaluate the policy instead. Which in the broadest sense is a fair question.
The problem with <para> as a whole is that many abuse it. If it's used as <para>This function does X</para>, it's mostly fine. But it's been used in many other things.

The docs are flooded with <para>s as an outer of <itemizedlist>. Sometimes with inner text, sometimes without which makes the docs unnecessary complicated for new contributors. They don't know when to use <para> or <simpara>.

It's kind of the same in PHP where this is valid, but not preferred because it adds additional nesting which over a longer time degrades readability.

if ($foo) {
   if ($bar) {
   }
} else {
   if ($foobar) {
   }
}

The main problem is, the whole docs are mixed. As you pointed out on Discord, the list of violations are 91.794. So very much ideally we would go over it once, perhaps with [skip-revcheck] and fix it in bulk. That way nobody needs to be bothered with it anymore. And docbook-cs only picks up violations that are actually caused by the author.

As for the author, docbook-cs in diff mode doesn't give all the violations of the changed file, but only the lines the author have changed and the direct parent. So in short, if the author changes the inner text of <para>, and as a result it could be converted into a <simpara> it will mark it as a violation.

Moreover, simpara renders differently (the first uses simpara, second para):

IMHO this has nothing to do with simpara on its own. It's due to the fact that it renders incorrectly as a <span class="simpara">. Which in should be properly fixed in PHD.

This PR php/phd#246 should fix that behaviour. I will try to review and merge it in the upcoming days.

[vrana]

The docs are flooded with <para>s as an outer of <itemizedlist>.

This rule doesn't address this issue. Exactly these problematic constructs will stay intact. The rule will "fix" only the places that need no fixing.

1. There's XYZ amount of <para><itemizedlist>.
2. Spend huge effort to replace all <para> with <simpara> where possible.
3. There's still XYZ amount of <para><itemizedlist>.

If we see <para> as an outer of <itemizedlist> problematic then there should be a rule for that instead.

[jordikroon]

Correct, but php/docbook-cs#17 does address that. Its impact both technical and on performance needs to be evaluated first. The CS fixer that Nick is working on could come handy when we add more rules along the way.

[vrana]

Which brings us back to my original point. There are some problematic constructs. This sniff doesn't prevent them. Other sniffs will prevent them. So there's no purpose in this sniff.

[jordikroon]

Why would this Sniff not have a purpose, if it follows the exact Docbook 5.2 spec. It suggests to use simpara where by spec it is allowed to be. It follows our Style Guide and is not problematic. For where it seems to be falsely problematic, is where PHD renders a span whereas it should be a p tag. That's a rendering error, not a sniff problem. Thus it should be fixed a PHD.

What this spec doesn't cover is exactly what I have mentioned, and there is a PR for that to cover it. It would not make sense to drop this Sniff just because. This Sniff by itself is complete.

The PR I did mention, suggests to omit the outer para where it's redundant.

[vrana]

I see no point neither in that style guide rule, nor in this sniff. I've asked many times what's the purpose of this rule in the style guide and this sniff. The purpose is not documented anywhere and nobody answered it to me.

The only answer I got is “People abuse <para>” which the style guide rule and this sniff does not prevent.

My point is that this sniff adds a lot of churn and doesn't solve any problem. Even running it on a diff triggers pre-existing errors. Copy-pasting text from elsewhere as well. If I run the CS on the whole manual, there's now thousands of errors which makes the full run unusable. Fixing the error in bulk would break translations. Also <para> is shorter and more natural to use. DocBook doesn't say that <simpara> is preferred.

What is all of this churn good for? Only to satisfy a styly guide rule which has no purpose.

[alfsb]

Changing all para to simpara to satisfy this rule would completely break how the docs is rendered and what was the intent of the author.

PhD is wrong here, full stop.

The Docbook manual defines simpara to be "Formatted as a displayed block." Formating it as sentences (HTML inlines) does not satisfy that.

[alfsb]

If it's used as <para>This function does X</para>, it's mostly fine.

So these should not be flagged. whereas ...

But it's been used in many other things. The docs are flooded with <para>s as an outer of <itemizedlist>. Sometimes with inner text, sometimes without which makes the docs unnecessary complicated for new contributors.

... these are to be flagged, instead.

It's a fair point to evaluate and eventually modify the police. Below is my take, from what I collected in this discussion, as for a starting point, comments, suggestions and to inspire other complete suggestions.

Note, whorever, this does not mandate the change in older texts that use <para>s for simple texts.

---

Paragraph markup

Use <simpara> in new texts where possible. That is, use <simpara> in text spans without inner block elements. Note, whorever, that <simpara> is currently rendered as sentences, not complete paragraphs.

Avoid using <para> to encompass other block elements, like <itemizedlist>. Prefer instead positioning <para> and <simpara> as adjacent to other block elements. For example, instead of:

 <para>
  text
  <itemizedlist>
   <listitem>
    <para>

Prefer:

 <simpara>text</simpara>
 <itemizedlist>
  <listitem>
   <simpara>text</simpara>

[alfsb]

This PR php/phd#246 should fix that behaviour. I will try to review and merge it in the upcoming days.

That PR asked me to review it, but running PhD is mostly out of reach for me :/

[vrana]

This sniff doesn't solve the issue with <para>text<itemizedlist>. It marks only completely fine <para>s with inline content. This serves no purpose.

Enforcing a style only on a new code is problematic for several reasons:

1. Copying the code from elsewhere will trigger a warning.
2. The new and old code will be inconsistent.
3. Running the checks on the whole codebase will have thousands of warnings making the full run unusable.
4. Trying to fix all the issues is problematic because of translations.

Given these reasons, new rules should be added only if they add a clear benefit. This sniff has no benefits - it marks only completely harmless code and doesn't mark code we consider as bad.

[Girgias]

Applying a rule only to new code or code that is being touched is extremely common in many projects, be that for static analysis or code style.

Considering that we will be moving a bunch of documentation code after https://wiki.php.net/rfc/third_party_ext_documentation, which I'm going to imagine will remove a bunch of the offending code, we can then start focusing on getting translations up to date so that one can update the files en masse (as I have done prior in the year) without using a skip revcheck and updating the revcheck on translations.

You personally not liking a code style is not a reason to get rid of a sniff.

Rendering issues are rendering issues that need to be fixed. Those are not an XML code style discussion.

Using the XML tag with the simplest possible semantics has benefits, you disagreeing with them does not mean you get to blatantly decide how stuff gets done, especially not when you're not one of the current main documentation maintainers.

[alfsb]

without using a skip revcheck

I think I fixed the problem of two consecutive skip-revcheks a few years ago, so using it on mass changes is now safe. It can be tested by taking the local revchecks of all translations, make the change and committing locally it with the skip-revchek mark in the first line of commit, and then regenerating all revchecks, to see if there are any differences, so...

updating the revcheck on translations.

... would not be necessary.

[vrana]

Applying a rule only to a new code creates a massive tech debt and is irresponsible. I've listed the problems of this approach above.

It doesn't matter if I agree with some code style or not. The problem with this sniff is that it doesn't address any real issue. It forces you to change <para> to <simpara> where it is harmless and ignores precisely the harmful <para>s.