Fix #749, typographical errors by JAuriac · Pull Request #746 · pdidev/pdi

JAuriac · 2026-08-26T14:50:29Z

Add github action for typographical errors

List of things to check before making a PR

Before merging your code, please check the following:

JAuriac · 2026-08-31T12:04:37Z

The only three files to review on this PR are :
.github/workflows/tests.yml, to use the 'typos' github action (similarly to ddc)
.typos.toml, to define exceptions for the 'typos' action
bin/test_indent, to handle removing trailing whitespaces and checking for correct newline at end of file
All the other files modified are typographical errors fixes

Yushan-Wang · 2026-09-04T10:24:46Z

+  spell:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v6
+    - uses: crate-ci/typos@v1


Thanks. This is quite useful for better code.
Could this be merged into the indent check?

Yes it could, they are left separated for now as to not extend the indent requirement over the rest of the CI (cf. "needs: second")
Should we merge those ?

I suggest incorporating typo checking inside indent, which will be the very first step of the CI workflow. Other CI steps will start only when the needs succeed.

Ok, change made
Another way to proceed would be to check this indent job last (first -> second -> rest of feature tests -> indent), as a way to the feature on all CI runs, and maybe only fix the indent step once the feature is ready to be merged

Ok, change made Another way to proceed would be to check this indent job last (first -> second -> rest of feature tests -> indent), as a way to the feature on all CI runs, and maybe only fix the indent step once the feature is ready to be merged

I think it is not optimal. In a case where only indent is not satisfied, CI will go through all tests -> indent to bring up the indent error. Then we fix the indent and the CI goes again with all tests -> indent to complete the cycle.
With the current setup, all tests will run only once, thus much cheaper

Yushan-Wang · 2026-09-04T10:28:15Z

+extend-exclude = [
+    "vendor/",
+    "pdi/docs/_template/",
+    "pdi/tests/PDI_expression.cxx",


Why is this file excluded?

It contains a test which itself checks for spelling mistake through dedicated lists, for example :
const vector<string> v_fpos{"y1", "YY", "yEs", "Yess", "YE5", "tru", "TrUe", "TRRUE", "onn", "0n", "ONN"};

I see.
When looking at the cxx file, at the very last part, this section

for (auto&& string_value: v_fneg) { ... ASSERT_EQ(PDI::Expression(string_value).to_long(context_mock), 1);

appears strange to me. Shouldn't it be comparing to 0 instead of 1?

It seems those values aren't required :

pdi/pdi/src/expression/impl/string_literal.cxx

Line 67 in 9e0d97e

long Expression::Impl::String_literal::to_long(Context& ctx) const

and that we need :
EXPECT_THROW(PDI::Expression(string_value).to_long(context_mock), PDI::Error);

Made PR #754 for issue #753 to fix this

isn't there a way to exclude specific lines with a comment in the file instead?

#746 (comment)
Added option to use specific comment to mark some lines or some code block as ignored by the 'typos' check

Yushan-Wang · 2026-09-04T10:31:53Z

+
+git ls-files -z | grep -zZv '^vendor/' | while IFS= read -r -d '' file; do
+  if [ -f "$file" ] && file --mime-type "$file" | grep -q 'text/'; then
+    sed -i -e 's/[ \t]*$//' -e '$a\' "$file"
+  fi
+done


Why this change?

This is to ensure having a line break at the end of each file, to prevent the issue of a line of code shown as modified through git if later another line is appended at the end of the file (also, this is required by the C standard)

https://github.com/CExA-project/ddc/blob/main/bin/newline_at_eof.py
Another PR will be dedicated to this, removing this change for this PR

jbigot · 2026-09-05T11:27:47Z

 					PC_tree_t data_name = PC_get(data_item, "{0}");
-					PC_tree_t datas = PC_get(data_item, ".%s", to_string(data_name).c_str());
-					opt_each(datas, [&](PC_tree_t one_data) {
+					PC_tree_t data = PC_get(data_item, ".%s", to_string(data_name).c_str());


I'm all in favor of fixing the doc & comments, but I'm not sure fixing the code with this is a very good idea. I'm afraid it might generate subtle bugs later.

Some variables might use a non English names because the correct English name is already used for something else. By "fixing" that, one might shadow the other variable & change behavior.

Ideally, wouldn't this issue be fixed by using more verbose variables names ?
Incidentally, relying on specific spellings would expose us to future discrepancies

I agree to check spelling for documentation and comments.

A new commit now changes the behaviour of the CI step to only issue warnings

jbigot

looks good overall

jbigot

good idea, but the end-of-line space check needs rework

jbigot · 2026-09-06T11:07:27Z

+git ls-files -z | grep -zZv '^vendor/' | while IFS= read -r -d '' file; do
+  if [ -f "$file" ] && file --mime-type "$file" | grep -q 'text/'; then
+    sed -i -e 's/[ \t]*$//' -e '$a\' "$file"
+  fi
+done


beware, this breaks the test completely!

The exit-code checked to verify if everything went fine in the CI used to be that of the last executed command, the clang-format one.

Now this comes after and completely discards the indentation test.

In addition, this does not work as a test and will never detect a typo. The last command is the loop command that has no reason to fail.

It does also not follow the logic of the command, it always applies the change, whether the --apply flag is passed or not.

Minor issues, but this uses GNU-specific sed extensions, I expect this to fail on MacOS.

Very minor issue, if any directory contains text/ in its path, everything below it will be checked.

https://github.com/CExA-project/ddc/blob/main/bin/newline_at_eof.py
Another PR will be dedicated to this, removing this change for this PR

jbigot · 2026-09-06T11:09:31Z

+[default.extend-words]
+ND = "ND"
+iy = "iy"
+iz = "iz"
+cpy = "cpy"


what's this about?

Those are considered typos by the tool, but not by us, so we define exceptions
"ND" designates N-dimensional arrays, and is a keyword mirroring "ndarray"
"iy" and "iz" are coordinates along specific dimensions (note that "ix", or "ny", "sy", "ry" are not caught by the tool)
"cpy" is mirroring the frequently used keyword of copy related to GPU use

jbigot · 2026-09-06T11:10:18Z

+extend-exclude = [
+    "vendor/",
+    "pdi/docs/_template/",
+    "pdi/tests/PDI_expression.cxx",


isn't there a way to exclude specific lines with a comment in the file instead?

JAuriac · 2026-09-08T08:21:01Z

#746 (comment)
Added option to use specific comment to mark some lines or some code block as ignored by the 'typos' check

jmorice91

In the puil_request_template.md, add a line for the typos CI tools as indent.

…n for typographical errors

iole-bolognesi

Looks good, I agree with Yushan's comment that it would be useful to add the ignore options to the How_to_create_plugin.md

JAuriac force-pushed the trailingAndSpelling branch 2 times, most recently from f8a9110 to 2be1551 Compare August 31, 2026 09:25

JAuriac marked this pull request as ready for review August 31, 2026 11:59

JAuriac requested a review from a team August 31, 2026 11:59

JAuriac requested review from Yushan-Wang, benoitmartin88 and jbigot as code owners August 31, 2026 11:59

JAuriac linked an issue Sep 4, 2026 that may be closed by this pull request

Add 'typos' action to github CI workflow #749

Open

JAuriac force-pushed the trailingAndSpelling branch from 3534fbd to c58d9fa Compare September 4, 2026 08:47

Yushan-Wang reviewed Sep 4, 2026

View reviewed changes

JAuriac force-pushed the trailingAndSpelling branch from c58d9fa to 86cd651 Compare September 4, 2026 14:30

jbigot reviewed Sep 5, 2026

View reviewed changes

Comment thread README.md Outdated

jbigot reviewed Sep 5, 2026

View reviewed changes

jbigot requested changes Sep 6, 2026

View reviewed changes

JAuriac changed the title ~~Trailing whitespaces & typographical errors~~ Typographical errors Sep 8, 2026

JAuriac changed the title ~~Typographical errors~~ Fix #749, typographical errors Sep 8, 2026

Yushan-Wang reviewed Sep 10, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Yushan-Wang reviewed Sep 10, 2026

View reviewed changes

Comment thread .typos.toml

JAuriac requested review from Yushan-Wang, iole-bolognesi, jbigot and jmorice91 September 14, 2026 08:45

jmorice91 reviewed Sep 14, 2026

View reviewed changes

Comment thread .github/workflows/tests.yml

Comment thread bin/test_indent Outdated

Comment thread CHANGELOG.md Outdated

JAuriac added 4 commits September 24, 2026 14:38

Add trailing whitespace handling to test_indent, and add github actio…

9417d40

…n for typographical errors

Add .toml configuration file for 'typos' action

4a5a674

Fix typographical errors, manual part [skip ci]

7855f53

Fix typographical errors, automatic part

07ace82

JAuriac added 5 commits September 24, 2026 14:38

Fix issue pdidev#749, satisfy pull_request_template.md

3d98ca8

Add feedback, remove EOF newline check

2c62150

Add feedback 2

557b187

Add feedback 3

e8e6285

Add feedback 4

6d2580c

JAuriac force-pushed the trailingAndSpelling branch from 5dad2c0 to 6d2580c Compare September 24, 2026 12:39

JAuriac requested a review from jmorice91 September 24, 2026 13:07

iole-bolognesi approved these changes Sep 24, 2026

View reviewed changes

Add feedback 5

dcab54c

Conversation

JAuriac commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List of things to check before making a PR

Uh oh!

JAuriac commented Aug 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JAuriac Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbigot left a comment

Choose a reason for hiding this comment

Uh oh!

jbigot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JAuriac commented Sep 8, 2026

Uh oh!

Uh oh!

Uh oh!

jmorice91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iole-bolognesi left a comment

JAuriac commented Aug 26, 2026 •

edited

Loading

JAuriac Sep 4, 2026 •

edited

Loading