Skip to content

Trim trailing whitespace and ensure Markdown files are POSIX compliant - #246

Merged
javagl merged 1 commit into
KhronosGroup:mainfrom
aaronfranke:posix-whitespace
Aug 21, 2026
Merged

Trim trailing whitespace and ensure Markdown files are POSIX compliant#246
javagl merged 1 commit into
KhronosGroup:mainfrom
aaronfranke:posix-whitespace

Conversation

@aaronfranke

@aaronfranke aaronfranke commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

This PR updates the PHP scripts to output POSIX-compliant text files, which means text files that end in one \n character. This PR also runs the scripts and manually goes over files the script did not touch (like source .gltf files). I also added an .editorconfig file to give a hint to IDEs that they should automatically fix this problem if encountered. This PR also trims trailing whitespace.

@javagl

javagl commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

With 659 modified files (including SVG and glTF), there's hardly a way to sensibly review this. I think that differentiating between the commit that changes the script, and one or multiple commits for the modified files, it could be clearer (so what exactly was the change to the PHP file that causes all these changes?)

But to start with a question waaaay higher: Where and how is this relevant?

@aaronfranke

Copy link
Copy Markdown
Contributor Author

@javagl There are many utilities that expect text files to be POSIX-compliant text files, especially on Mac/Linux systems. For example, cat will behave incorrectly in those cases, with the next terminal prompt appearing on the same line as the file's "final line" (in quotes because, by POSIX standard, a line of a text file ends in \n, therefore any bytes following the final \n are extraneous characters not on any line of the text file per the standard). For example, GitHub itself will display warnings when text files do not end in a \n character. For example, C/C++ compilers will complain about this: older compilers had it as an error, newer ones have it as a warning, but still if you compile with warnings treated as errors then it won't compile. This repo isn't C/C++ code, but still, many users including myself have their IDEs configured to automatically fix this problem on save. So, to the point, having POSIX-compliant text files in every repo makes it easier to work with, for me and anyone else who has their IDE configured like me. Otherwise I either need to reconfigure my IDE temporarily, or manually discard changes in Git. Aside from that, I would argue having files consistent is beneficial for its own sake, and consistent with a standard is even better. Formatting is too important to ignore, but not important enough to spend time on, so it's good to have easy-to-follow rules.

As for reviewing, click the "Hide whitespace" check box in the "Files Changed" tab to make reviewing easier:

Screenshot 2025-11-14 043345

Comment thread util/modelmetadata.php Outdated
@@ -504,10 +511,10 @@ public function writeReadme ($tagListings=null) {
$readme[] = $this->metadata['credit'][$ii];
}
//$readme[] = "#### Assembled by " . AppName . ' ' . AppVersion;
$readme[] = "#### Assembled by " . AppName;
$readme[] = "#### Assembled by " . AppName . "\n";

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are 3 total lines of the PHP script changed to specifically add the \n, here is one case.

Comment thread util/modelmetadata.php Outdated
'link'=>'',
'text'=>'Cesium Trademark or Logo',
'spdx'=>'LicenseRef-LegalMark-Cesium',
),
'LicenseRef-LegalMark-DGG' => array (

@aaronfranke aaronfranke Nov 14, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I forgot I included these, I put these changes in here because otherwise the license in .reuse/dep5 gets generated as License: CC-BY-4.0 AND LicenseRef-LegalMark-Khronos AND with a trailing space and a trailing AND. But that should probably get another PR to fix this first. EDIT: So let's merge #248 first.

@javagl

javagl commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

I'm aware of the constant background noise of \n vs \r\n , and there's even that Git autocrlf thingy that tries to avoid the confusion of choosing between two things by offering three options 🤪 . But I wasn't aware that there's an actual "issue" with the current state of this repo.

As a baseline, I'm skeptical for a change like this. So there was something in the .gltf files that was changed here. Imagine someone notices "Oh, model X should be green instead of red", and fixes that, and writes out a new version of that model, as a .gltf, with whatever tool that user is using. This .gltf file will then have again the "issue" that is fixed here.

The main point is that changing 659 files warrants some scrutiny. This is not C++ code, and when it's only about some IDE inserting an extra line, then this sounds like a configuration issue of the IDE, and not something that warrants such a wide-ranging change.

("Removing trailing whitespaces in .md" or so could be OK, but there are dedicated linters for that, and it would not affect the .gltf files...)

@aaronfranke
aaronfranke marked this pull request as draft November 14, 2025 14:53
@emackey

emackey commented Jan 30, 2026

Copy link
Copy Markdown
Member

Removing trailing whitespaces in .md

Actually, a pair of trailing spaces in Markdown is often intentional. It was the original official Markdown way of adding a line break, before <br/> was allowed in Markdown. We used to use these, for example in the CC0 messages in the old glTF-Sample-Models repo (now archived).

https://www.markdownguide.org/basic-syntax/#line-break-best-practices

I've added some end-of-file fixes in #254. Perhaps this PR can be closed?

@aaronfranke

Copy link
Copy Markdown
Contributor Author

For Markdown line breaks, I just opened PR #265, which should be merged ahead of this PR.

@aaronfranke
aaronfranke force-pushed the posix-whitespace branch 2 times, most recently from 925f864 to ae2ae44 Compare February 3, 2026 03:15
@aaronfranke
aaronfranke marked this pull request as ready for review February 4, 2026 19:44
@aaronfranke aaronfranke changed the title Trim trailing whitespace and ensure text files are POSIX compliant Trim trailing whitespace and ensure Markdown files are POSIX compliant Aug 21, 2026
@aaronfranke
aaronfranke requested a review from javagl August 21, 2026 15:18

@javagl javagl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Summarizing some discussion (also from the Tooling TSG call):

There had been some concerns about formatting .gltf and .svg files in a particular way. Every time that a .gltf was updated (or a new one was added), the state would "diverge" again from the current state here.

One thing that would make sense and still can be considered:
We would like to have some human-readable formatting for the .gltf files. People shouldn't submit .gltf files with { everything: { in: { one: line }}} for example. This cannot sensibly be made a requirement for submitting and merging a PR (because we don't have influence on the tools that generate these files). But it could be made part of CI: Whenever a PR is merged, the CI could do some trivial pseudocode of
write(read("file.gltf"), "file.gltf");
to apply consistent formatting to the glTF files.


That said, this PR was now updated to only affect the markdown files, and specifically the body.md files. This is important: These are the files that are contributed externally - in contrast the the actual README.md for each model, which is automatically generated, and only inlines the respective .body.md file.

So this PR brings all the body.md files into a consistent shape. Looking at the changes, it is indeed only

  1. removing unnecessary trailing spaces
  2. removing unnecessary newlines
  3. adding newlines at the end of the files where they are due

So I think that this can be merged.


There had been some inlined comments about the PHP, e.g. at #246 (comment) . Given that the PHP was changed to TypeScript as of #280 , I assume that these comments are obsolete. This involves the (somewhat bold) assumption that the new, TypeScript based tools generate MD that does not have the issues that have been fixed here. But this may have to be confirmed. Which leads to the last point:

There should probably be some CI step for (automated) Markdown formatting. I've opened #290 for tracking this.

@javagl

javagl commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

So I'll just go ahead and merge this.

@aaronfranke If you want to address #290 , just drop me a note. Otherwise, I'll try to have a look at this soon (before there are too many additions that "diverge" from the current state)

@javagl
javagl merged commit 66858e7 into KhronosGroup:main Aug 21, 2026
2 checks passed
@aaronfranke
aaronfranke deleted the posix-whitespace branch August 22, 2026 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants