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.

Sign up for GitHub

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

Jump to bottom

Enforce para as first child in step #68

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enforce para as first child in step #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 36 additions & 33 deletions geekodoc/rng/geekodoc5-flat.rnc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,38 +162,39 @@ div {
| db.extension.blocks
| db.admonition.blocks)*
db.step.blocks =
db.para.blocks
| db.remark
| db.indexterm
| # TODO: Remove that (currently we have too much of that)
db.example
| db.figure
| db.informalexample
| db.informalfigure
| db.informaltable
| db.admonition.blocks
| db.graphic.blocks
| db.verbatim.blocks
| db.extension.blocks
| db.itemizedlist
| db.orderedlist
| db.simplelist
| db.variablelist
| db.calloutlist
| db.calloutlist
| db.annotation
| db.xi.include
| db.table
# no db.publishing.blocks, db.technical.blocks
# | db.formal.blocks - db.table - db.equation
# | db.informal.blocks - db.informaltable - db.informalequation
# | db.procedure
# | db.segmentedlist
# | db.glosslist
# | db.bibliolist
# | db.qandaset
# - - - -

(db.para.blocks
| db.remark
| db.indexterm
| # TODO: Remove that (currently we have too much of that)
db.example
| db.figure
| db.informalexample
| db.informalfigure
| db.informaltable
| db.admonition.blocks
| db.graphic.blocks
| db.verbatim.blocks
| db.extension.blocks
| db.itemizedlist
| db.orderedlist
| db.simplelist
| db.variablelist
| db.calloutlist
| db.calloutlist
| db.annotation
| db.xi.include
| db.table
| db.screen)
# no db.publishing.blocks, db.technical.blocks
# | db.formal.blocks - db.table - db.equation
# | db.informal.blocks - db.informaltable - db.informalequation
# | db.procedure
# | db.segmentedlist
# | db.glosslist
# | db.bibliolist
# | db.qandaset
# - - - -
*
}
div {
db.xlink.hrefreq.simple.link.attributes =
Expand Down Expand Up @@ -10876,7 +10877,9 @@ div {
element step {
db.step.attlist,
# db.step.info?,
((db.step.blocks+,
db.remark?,
db.para,
((db.step.blocks?,
((db.substeps | db.stepalternatives), db.step.blocks*)?,
db.result?)
| ((db.substeps | db.stepalternatives),
Expand Down
15 changes: 11 additions & 4 deletions geekodoc/rng/geekodoc5.rnc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ db.entry.model =


db.step.blocks =
db.para.blocks
( db.para.blocks
| db.remark
| db.indexterm # TODO: Remove that (currently we have too much of that)
| db.example
Expand All @@ -138,6 +138,7 @@ db.step.blocks =
| db.annotation
| db.xi.include
| db.table
| db.screen
# no db.publishing.blocks, db.technical.blocks
#| db.formal.blocks - db.table - db.equation
#| db.informal.blocks - db.informaltable - db.informalequation
Expand All @@ -147,6 +148,7 @@ db.step.blocks =
#| db.bibliolist
#| db.qandaset
# ----
)*
}

div {
Expand Down Expand Up @@ -1507,12 +1509,17 @@ include "docbookxi.rnc"
element step {
db.step.attlist,
# db.step.info?,
((db.step.blocks+,
db.remark?,
db.para,
(
(db.step.blocks?,
((db.substeps | db.stepalternatives), db.step.blocks*)?,
db.result?)
| ((db.substeps | db.stepalternatives),
|
((db.substeps | db.stepalternatives),
db.step.blocks*,
db.result?))
db.result?)
)
}
}

Expand Down
11 changes: 11 additions & 0 deletions geekodoc/tests/bad/article-procedure-step-para.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?xml version="1.0" encoding="UTF-8"?>
<?xml-model href="../../rng/geekodoc5-flat.rnc" type="application/relax-ng-compact-syntax"?>
<article xmlns="http://docbook.org/ns/docbook"
xmlns:xlink="http://www.w3.org/1999/xlink" version="5.0">
<title>Procedure: para as first child of step</title>
<procedure>
<step>
<screen/>
</step>
Copy link

Choose a reason for hiding this comment

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

In some cases, this kind of step/screen pattern may be appropriate. E.g. when you're working on just a cheat sheet or so. (Though I am not sure we want to cover cheat sheets with Geekodoc.)

Copy link
Contributor

@taroth21 taroth21 Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

OK, for a cheat sheet, a step with only a screen might make sense. But for 98% of the cases within the document types we currently have in our portfolio, I would still say that a screen usually should be accompanied by a para and should not be the only element within a step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... I have to admit, I didn't think of cheat sheets. However, as Tanja already explained, we don't have that ATM. So I think, we shouldn't cover this use case.

I would propose to change it accordingly (e.g. enforce para as a first child). Even if you want to write cheat sheets, you could do that with an empy <para/>. I know, it's a bit more verbose, but in case we really need to write cheat sheets, we could revisit this issue.

Copy link

Choose a reason for hiding this comment

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

  1. I see your point -- essentially GeekoDoc is not really made for cheatsheets.
  2. From a perspective of not breaking too many existing documents at a time, I would (still) somewhat favor allowing step/screen[1] - I am pretty sure this will be an issue with current SES, CaaS P and CAP docs.

In any case, we should not ask people to use empty <para/>s. That is just terrible practice and will only make people wonder how many other things they can work around in a similar way. It can also lead to layout issues (like empty lines).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the empty <para/> wasn't a great idea.

If you favor step/screen[1] as an additional first element (the correct XPath would be step/*[1][self::screen], but anyway JFYI), wouldn't this issue be pointless? 😉

I'm a bit confused now as your suggestion makes the change kind of obsolete, right? Just to compare, this is the original definition of a step:

db.step =
    ## A unit of action in a procedure
   element step {
      db.step.attlist,
      db.step.info,
      ((db.all.blocks+,
        ((db.substeps | db.stepalternatives), db.all.blocks*)?,
        db.result?)
       | ((db.substeps | db.stepalternatives),
          db.all.blocks*,
          db.result?))

And here is the definition in GeekoDoc:

db.step =
    ## A unit of action in a procedure
    element step {
      db.step.attlist,
      # db.step.info?,
      db.remark?,
      db.para,
      ((db.step.blocks?,
        ((db.substeps | db.stepalternatives), db.step.blocks*)?,
        db.result?)
       |  ((db.substeps | db.stepalternatives),
          db.step.blocks*,
          db.result?)
      )      
    }

As you can see, it's quite similar. The only difference is db.step.blocks vs. db.all.blocks and the db.remark and db.para as the first child elements.

If we remove that, the whole pattern will be useless. 😉 I don't have anything against that. If we want to be compatible with existing documentation, I can remove the step customization. In that case, we should close this PR and the related issue.

What do you think?

Copy link

Choose a reason for hiding this comment

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

I'm a bit confused now as your suggestion makes the change kind of obsolete, right?

Afai remember, the reason for this issue was that people (CaaSP writers specifically) were using (in)formalfigure as the first element and complaining that it does not look good. To which we/I said, "sure, but it also makes no sense to do that". I don't think screen usage was the issue we were concerned with. Then again, in a sense, you are right -- screen is not too different from figure in the sense that it does not give proper context for a step. I guess I ultimately agree that it probably should not be allowed in first position either.

</procedure>
</article>
15 changes: 15 additions & 0 deletions geekodoc/tests/bad/procedure-informalfigure-in-step.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?xml version="1.0" encoding="UTF-8"?>
<?xml-model href="../../rng/geekodoc5-flat.rnc" type="application/relax-ng-compact-syntax"?>
<procedure xmlns="http://docbook.org/ns/docbook"
xmlns:xlink="http://www.w3.org/1999/xlink" version="5.1">
<title>A procedure with an invalid step</title>
<step>
<informalfigure>
<mediaobject>
<imageobject>
<imagedata fileref="just-an-image.png"/>
</imageobject>
</mediaobject>
</informalfigure>
</step>
</procedure>
6 changes: 4 additions & 2 deletions geekodoc/tests/good/book.storage.admin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?xml version="1.0"?>
<?xml-model href="../../rng/geekodoc5-flat.rnc" type="application/relax-ng-compact-syntax"?>
<book xmlns="http://docbook.org/ns/docbook" xmlns:xi="http://www.w3.org/2001/XInclude" xmlns:xlink="http://www.w3.org/1999/xlink" xml:base="book_storage_admin.xml" version="5.0" xml:lang="en" xml:id="book.storage.admin">
<info>
<title>Administration and Deployment Guide</title><productname>SUSE Enterprise Storage</productname><productname role="abbrev">SES</productname>
Expand Down Expand Up @@ -2892,8 +2893,7 @@ screen install-ses-admin</screen>
</para>

<procedure>
<step>
<warning>
<warning>
<title>Do Not Run <command>zypper dup</command> or Reboot the Node</title>
<para>
After you prepare for the upgrade to SUSE Linux Enterprise Server 12 SP2 as suggested later in
Expand All @@ -2902,6 +2902,7 @@ screen install-ses-admin</screen>
correctly.
</para>
</warning>
<step>
<para>
Upgrade the current SUSE Linux Enterprise Server to version 12 SP2. Refer to
<link xlink:href="https://www.suse.com/documentation/sles-12/book_sle_deployment/data/cha_update_sle.html"/>
Expand Down Expand Up @@ -11018,6 +11019,7 @@ Superuser created successfully.
</para>
</step>
<step>
<para/>
<tip>
<para>
In order for Calamari to work correctly, the admin keyring needs to be
Expand Down
42 changes: 42 additions & 0 deletions geekodoc/tests/good/procedure-step.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?xml version="1.0" encoding="UTF-8"?>
<?xml-model href="../../rng/geekodoc5-flat.rnc" type="application/relax-ng-compact-syntax"?>
<procedure version="5.1"
xmlns="http://docbook.org/ns/docbook"
xmlns:xlink="http://www.w3.org/1999/xlink">
<title>A procedure with valid steps</title>
<step>
<remark>Just a remark before a para</remark>
<para>A</para>
</step>
<step>
<para>B</para>
<remark>A remark after the para</remark>
</step>
<step>
<para>C</para>
<result>
<para/>
</result>
</step>
<step>
<para>D</para>
<screen/>
<substeps>
<step>
<para/>
</step>
<step>
<para/>
</step>
</substeps>
<para></para>
</step>
<step>
<para>E</para>
<itemizedlist>
<listitem>
<para/>
</listitem>
</itemizedlist>
</step>
</procedure>