Run reflow #481

TApplencourt · 2023-10-25T19:47:01Z

Update our reflow.py copy and run it.

Link: https://github.com/KhronosGroup/Vulkan-Docs/blob/main/scripts/reflow.py and thanks @oddhack :)

This does what we wanted:

Force a maximum line width (default 76 char)
Split on period
Delete 2+ contiguous spaces

This is a small PR, so it should be easy to review...

gmlueck

Wow, this is great! There are a couple problems I point out below, but I think they can be easily fixed.

To validate that nothing got inadvertently changed, I built the PDF both before and after this change and compared the PDF versions. There were no unexpected changes. (There were some expected changes on the title page related to the git version and the build date.)

To people who were not at lunch today, this addresses #472. Thomas was noting how this could be easily done via a script, so I guess he volunteered!

adoc/chapters/acknowledgements.adoc

adoc/chapters/device_compiler.adoc

adoc/chapters/opencl_backend.adoc

oddhack · 2023-10-26T01:58:06Z

A few other notes about this script:

Requires pretty strict adherence to style guidelines, because it's not running the asciidoctor processing pipeline and does not know about every possible asciidoc construct. When I initially did the SYCL format conversion I followed the Vulkan style guidelines in general, so it's not surprising this basically works for you. The Vulkan style guide contains a lot of stuff that is irrelevant to SYCL but much of the markup / writing / miscellaneous sections may be worth extracting.
Will not reflow long lines in captions and titles (not supported by asciidoc), nor inside tables (possible, but $TOOCOMPLEX for the minimal upside).

It is pretty robust within the scope of what it's intended for. We use it frequently on Vulkan.

oddhack · 2023-10-26T01:59:57Z

@gmlueck it is an orthogonal issue, but you may want to review the Khronos spec copyright and make sure that's up to date with the latest version Neil has (V10, I think?).

Some Asciidoc references to the glossary terms were using the name of the glossary term instead of the link name. For glossary terms that have multiple words, this results in a reference that has a space in the name. This space is causing problems in KhronosGroup#481. All these glossary terms have an anchor that is the same as the glossary term, but with a hyphen instead of the space that separates the words. Change the references to use this anchor name instead of using the glossary term.

TApplencourt · 2023-10-26T15:14:56Z

Will rebase with #482 after it will be merged.

keryell

Looks globally like a good idea.
Probably you can rephrase "Keep a maximum line width," in the description to fit the mathematical property you want to express.

keryell · 2023-11-02T15:38:41Z

@oddhack do you know why 76 characters in "Force a maximum line width (default 76 char)"?
Why not 79? I understand that 80 is a little bit brittle for some hardware differences about how is handled the real CR.

oddhack · 2023-11-02T15:48:14Z

@oddhack do you know why 76 characters in "Force a maximum line width (default 76 char)"? Why not 79?

Historical dating back to OpenGL extension days - use whatever is convenient to you. If you use 999999 you get one sentence/line :-) Just make sure you like the value you start with so you don't generate lots of git churn by changing it later.

keryell · 2023-11-02T22:16:58Z

I mean, if we agree on 80 columns inherited from punch cards which percolated for decades into hardware terminals quite before OpenGL and computer graphics, why 76? You loose 4 characters... :-)

TApplencourt · 2023-11-03T15:34:50Z

@gmlueck Added a CI test.

TApplencourt · 2023-11-03T18:01:20Z

And perfect, the validation script work (https://github.com/KhronosGroup/SYCL-Docs/actions/runs/6748691663/job/18347563925)

TApplencourt · 2023-11-03T18:13:02Z

This is ready to merge (if we agree on line width...). Summary of changes:

Copied Vulkan reflow.py and dependencies in our repo + updated abbrev list (thanks again @oddhack)
Run reflow on all chapters
Added a CI script to verify chapters conformance with reflow
Minor: Updated some github receipt version

gmlueck · 2023-11-03T20:18:31Z

What column width are you enforcing now? Is it 76 or 80? I agree with @keryell, 80 makes more sense than 76. From @oddhack's response, it didn't seem like there was any particular reason to choose 76.

nliber · 2023-11-03T20:26:45Z

80 columns makes sense -- if folks are still using ADM-5 terminals! Even the mighty VT-100 had a 132 column mode.

Note: 80 columns is too many for my TRS-80, which only has 64 columns. (Okay, it also only had ALL CAPS, at least until I mod-ed it...)

(Honestly, just pick something...)

TApplencourt · 2023-11-03T20:28:38Z

The default. So it's 76. Agree that 80 is the most popular. black -- the python formater -- did choose 88 🤷🏽

You probably noticed the peculiar default line length. Black defaults to 88 characters per line, which happens to be 10% over 80. This number was found to produce significantly shorter files than sticking with 80 (the most popular), or even 79 (used by the standard library). In general, 90-ish seems like the wise choice.

But they are talking about code length, not text. If we have already two people pushing for 80, I can change to that. Don't want to bikeshedding it too much :)

TApplencourt · 2023-11-03T20:34:36Z

Changed default to 80. Funnily enough, it doesn't change anything :)

keryell · 2023-11-03T20:53:47Z

The reason I propose 79 instead of 80 is that sometimes 80 is a problem when there is a 80-hard limit and some editors or modes display some EOL char at the end, like my Emacs whitespace-mode to show all the weird chars.

nliber · 2023-11-03T20:57:15Z

Doesn't making it more than 72 columns make us incompatible with Fortran?

(Sorry; couldn't resist...)

ProGTX

LGTM, though I wonder if we need all of the Vulkan related stuff?

Also I agree with the 80 column limit, it's the most standard.

adoc/scripts/vkconventions.py

oddhack · 2023-11-07T06:01:33Z

.github/workflows/CI.yml

@@ -40,8 +40,11 @@ jobs:
        run: |
          cd adoc
          make OUTDIR=/tmp/out QUIET= html pdf
+      - name: Verify reflow conformance
+        run: |
+          ./adoc/scripts/verify_reflow_conformance.sh


If I'm reading the script right, it's requiring that running reflow does nothing? That seems likely to make essentially every PR fail CI until you condition people to do this whenever they make a spec update. I've found it easier as the Vulkan spec editor to just do this when integrating PRs.

If I'm reading the script right, it's requiring that running reflow does nothing?

Yes. If I remember correctly, it was an ask from @gmlueck .

It looks like GitHub support issuing warning if we want to go this way.

Why is is bad for CI to fail if "reflow" detects that the formatting is wrong? This will prevent PRs from being merged until the formatting is fixed. Actually, I am hoping that contributors learn to run "reflow" themselves and fix these problems. However, even if the spec editor ends up doing it, the failing CI will ensure that someone fixes the formatting before PRs are merged.

Instead of pulling in all the Vulkan conventions.

oddhack · 2023-11-07T06:18:51Z

@TApplencourt I made a PR against this branch on your fork that just implements the required SYCL conventions for the script to succeed.

Only implement necessary parts of a SYCL conventions object

gmlueck

The reason I propose 79 instead of 80 is that sometimes 80 is a problem when there is a 80-hard limit and some editors or modes display some EOL char at the end, like my Emacs whitespace-mode to show all the weird chars.

If 79 makes life easier for Emacs users, we can use that. This PR hasn't changed the column width yet, so it's still easy to choose a different number. Let's try to decide today, though.

@TApplencourt, there are a number of other PRs that are ready to merge, so I propose the following order:

We merge whatever PRs the WG approves in the meeting today.
You merge latest SYCL-2020/master branch into this PR.
Run reflow with whatever margin limit we decide on (79 or 80).
I'll merge this PR as an editorial change before the next WG meeting.

adoc/scripts/reflow.py

gmlueck · 2023-11-10T21:51:38Z

@TApplencourt, other PRs have all been merged now, so we are ready for you to merge the latest "SYCL-2020/master" into this PR, resolve conflicts, and rerun reflow.

I suspect the merge conflicts might be substantial. If it is easier for you, you could create a new PR with the reflow scripts and run reflow in that new PR.

However you do it, I'll do a final check of the PDF to make sure nothing gets accidentally changed before merging.

TApplencourt · 2023-11-10T23:01:59Z

Please see #495

Run reflow

9e6a0d5

TApplencourt added the editorial Some purely editorial problem label Oct 25, 2023

gmlueck reviewed Oct 25, 2023

View reviewed changes

adoc/chapters/acknowledgements.adoc Outdated Show resolved Hide resolved

adoc/chapters/device_compiler.adoc Outdated Show resolved Hide resolved

adoc/chapters/opencl_backend.adoc Outdated Show resolved Hide resolved

gmlueck mentioned this pull request Oct 26, 2023

Use hyphens in reference names #482

Merged

keryell approved these changes Oct 31, 2023

View reviewed changes

TApplencourt added 2 commits November 1, 2023 11:06

Update reflow.py from vulkan

6d7cd9f

Add co and ltd as abbrev

1da369b

TApplencourt force-pushed the reflow branch 3 times, most recently from 874620c to a1cf91e Compare November 3, 2023 15:17

Add CI

81cf97d

TApplencourt force-pushed the reflow branch from a1cf91e to 81cf97d Compare November 3, 2023 15:25

TApplencourt added 2 commits November 3, 2023 10:26

Remove __pycache__

200c498

Fix typo CI

6d10711

Merge branch 'SYCL-2020/master' into reflow

a07b94b

Rerun reflow

07525b4

Default 80

aa7f7a4

ProGTX approved these changes Nov 6, 2023

View reviewed changes

adoc/scripts/vkconventions.py Outdated Show resolved Hide resolved

oddhack reviewed Nov 7, 2023

View reviewed changes

Only implement necessary parts of a SYCL conventions object

8e71887

Instead of pulling in all the Vulkan conventions.

Merge pull request #1 from oddhack/reflow-sycl

96368f1

Only implement necessary parts of a SYCL conventions object

gmlueck mentioned this pull request Nov 9, 2023

Refine vec interface code snippet #479

Merged

gmlueck reviewed Nov 9, 2023

View reviewed changes

adoc/scripts/reflow.py Outdated Show resolved Hide resolved

Default to 80

c3cf71f

gmlueck mentioned this pull request Nov 9, 2023

Clarify sections previously using "synchronize" #484

Merged

Merge with master

fc7098f

TApplencourt mentioned this pull request Nov 10, 2023

Reflow: break lines at end of sentence / 80 cols #495

Merged

TApplencourt closed this Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run reflow #481

Run reflow #481

TApplencourt commented Oct 25, 2023 •

edited

Loading

gmlueck left a comment

oddhack commented Oct 26, 2023

oddhack commented Oct 26, 2023

TApplencourt commented Oct 26, 2023

keryell left a comment

keryell commented Nov 2, 2023

oddhack commented Nov 2, 2023

keryell commented Nov 2, 2023

TApplencourt commented Nov 3, 2023

TApplencourt commented Nov 3, 2023

TApplencourt commented Nov 3, 2023

gmlueck commented Nov 3, 2023

nliber commented Nov 3, 2023 •

edited

Loading

TApplencourt commented Nov 3, 2023 •

edited

Loading

TApplencourt commented Nov 3, 2023 •

edited

Loading

keryell commented Nov 3, 2023

nliber commented Nov 3, 2023

ProGTX left a comment

oddhack Nov 7, 2023

TApplencourt Nov 7, 2023

gmlueck Nov 7, 2023

oddhack commented Nov 7, 2023

gmlueck left a comment

gmlueck commented Nov 10, 2023

TApplencourt commented Nov 10, 2023

Run reflow #481

Run reflow #481

Conversation

TApplencourt commented Oct 25, 2023 • edited Loading

gmlueck left a comment

Choose a reason for hiding this comment

oddhack commented Oct 26, 2023

oddhack commented Oct 26, 2023

TApplencourt commented Oct 26, 2023

keryell left a comment

Choose a reason for hiding this comment

keryell commented Nov 2, 2023

oddhack commented Nov 2, 2023

keryell commented Nov 2, 2023

TApplencourt commented Nov 3, 2023

TApplencourt commented Nov 3, 2023

TApplencourt commented Nov 3, 2023

gmlueck commented Nov 3, 2023

nliber commented Nov 3, 2023 • edited Loading

TApplencourt commented Nov 3, 2023 • edited Loading

TApplencourt commented Nov 3, 2023 • edited Loading

keryell commented Nov 3, 2023

nliber commented Nov 3, 2023

ProGTX left a comment

Choose a reason for hiding this comment

oddhack Nov 7, 2023

Choose a reason for hiding this comment

TApplencourt Nov 7, 2023

Choose a reason for hiding this comment

gmlueck Nov 7, 2023

Choose a reason for hiding this comment

oddhack commented Nov 7, 2023

gmlueck left a comment

Choose a reason for hiding this comment

gmlueck commented Nov 10, 2023

TApplencourt commented Nov 10, 2023

TApplencourt commented Oct 25, 2023 •

edited

Loading

nliber commented Nov 3, 2023 •

edited

Loading

TApplencourt commented Nov 3, 2023 •

edited

Loading

TApplencourt commented Nov 3, 2023 •

edited

Loading