Fix `bin.addheader` bug causing crash on modules with `%s` in module body by jfeo · Pull Request #682 · ucphhpc/migrid-sync

jfeo · 2026-09-30T11:26:28Z

Fix bin.addheader bug causing it to crash on modules with %s anywhere
in the module body.

Also adds fundamental testing of the bin.addheader.main function, and some
support functions in tests.support.iosupp for working reading and writing files
and filetrees in tests.

…anywhere in the module body Also adds fundamental testing of the `bin.addheader.main` function, and some support functions for working with io

jonasbardino

The fix looks right and good idea to add unit tests while at it.

A number of GH actions failed so I'd be nice if you could either look into fixing or commenting on them before I proceed with the review. We have seen false positives e.g. from unrelated lint warnings in the past but it doesn't look like just that here.

…m bin.addheader import`

jfeo · 2026-10-02T08:20:25Z

The fix looks right and good idea to add unit tests while at it.

A number of GH actions failed so I'd be nice if you could either look into fixing or commenting on them before I proceed with the review. We have seen false positives e.g. from unrelated lint warnings in the past but it doesn't look like just that here.

I'm investigating this. I ran the jobs locally with act successfully. It appears that checkout/v4 falls back to using the GitHub REST API because rocky9/rocky10 has an old version of git, and I wonder if that could be related to the import issue.

jfeo · 2026-10-02T08:43:25Z

The fix looks right and good idea to add unit tests while at it.
A number of GH actions failed so I'd be nice if you could either look into fixing or commenting on them before I proceed with the review. We have seen false positives e.g. from unrelated lint warnings in the past but it doesn't look like just that here.

I'm investigating this. I ran the jobs locally with act successfully. It appears that checkout/v4 falls back to using the GitHub REST API because rocky9/rocky10 has an old version of git, and I wonder if that could be related to the import issue.

I found the reason.

The issue is that checkout/v4 under Rocky9/Rocky10 (git version < 2.18) fall back to the GitHub Rest API and downloads an archive tarball, which respects the export-ignore rule for bin/addheader.py in .gitattributes

…y in rocky9/rocky10 CI This is done because the CI `checkout` action falls back to using the GitHub REST API when run under Rocky9/Rocky10, and the REST API respects `.gitattributes`, thus excluding `bin/addheader.py`.

jonasbardino · 2026-10-02T10:12:16Z

+def add_header(modulename: str, modulebody: str) -> str:
+    """Naive function to add a header, used for generting test data."""
+    return (
+        HEADER_FORMAT.format(modulename=modulename, description=DEFAULT_DESC)


We have opted for percent-expansion over other string construction methods so far. I think we need to discuss in the dev team whether or when to stray from that.

Alright. Should I change this printf-style formatting for now then?

The path of least resistance for merging is going with percent expansion. You can still use the dict form to preserve named values.
Feel free to branch the current version with the other review issues addressed and save it for a possible follow up PR only to switch to .format after we have discussed it in the team next week(?)

jonasbardino · 2026-10-02T10:12:56Z

+TreeDict = Union[dict[str, "TreeDict"], dict[str, str]]
+
+
+def write_file(directory: str, name: str, content: str) -> None:


I think we want to discuss strict typing in the dev team before we venture too far down the path of using it.

Okay. So far I have opted to add it when adding or modifying code, since it can co-exist with untyped code.

Yes, I noticed it in other PRs too and while I'm not against the idea and it doesn't hurt as such it still potentially introduces inconsistency. So I still think it's something we want to briefly discuss with pros and cons weighed in the dev team before we decide the path ahead.

jonasbardino · 2026-10-02T10:13:09Z

+import os
+import shutil
+import tempfile
+import unittest


minor: unittest import appears unused now

jonasbardino · 2026-10-02T10:14:28Z

+        self.target_dir = tempfile.mkdtemp(prefix=self.id())
+
+    def after_each(self) -> None:
+        shutil.rmtree(self.target_dir)


Hmm, I think I recall that we wipe test locations automatically in other cases to avoid this kind of explicit post-test cleanup, but I might be wrong or perhaps it's specific to the new iosupp module. In any case it would be nice to avoid because it's easy to forget.

jonasbardino · 2026-10-02T10:15:05Z

+import unittest
+
+from bin.addheader import __file__ as addheader_module_file
+from bin.addheader import main


nit-pick: should we keep all imports from one module in a single import ?

jonasbardino · 2026-10-02T10:15:16Z

+def write_tree(root: str, tree: TreeDict) -> None:
+    """
+    Write the given given tree to the given root directory.
+    """


nit-pick: is this black formatting or on purpose? I'd expect shorter than 80 char doc-strings to remain on a single line as PEP8 recommends.

jonasbardino · 2026-10-02T10:16:29Z

+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software Foundation,
+# Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#


nit-pick: this header looks oddly under-filled (significantly shorter than 80 chars). Did you use addheader to generate it or do we have those same overly short lines in another module you reused from?
Same applies to similar occurrences in other files below.

jonasbardino · 2026-10-02T10:20:31Z

+
+
+def add_header(modulename: str, modulebody: str) -> str:
+    """Naive function to add a header, used for generting test data."""


nit-pick: typo in generating

jonasbardino · 2026-10-02T10:22:26Z

+
+def write_tree(root: str, tree: TreeDict) -> None:
+    """
+    Write the given given tree to the given root directory.


nit-pick: duplicate 'given'

jonasbardino

Looks good overall, but I think we need to sort out a few things as mentioned in the comments. Also has minor bits and pieces to polish while at it.

Fix bin.addheader bug that caused it to crash on modules with %s …

a7c8165

…anywhere in the module body Also adds fundamental testing of the `bin.addheader.main` function, and some support functions for working with io

jfeo requested a review from a team September 30, 2026 11:27

jfeo self-assigned this Sep 30, 2026

jfeo added the bug Something isn't working label Sep 30, 2026

jonasbardino reviewed Sep 30, 2026

View reviewed changes

Comment thread bin/addheader.py

jonasbardino reviewed Sep 30, 2026

View reviewed changes

Try to fix import error errors on CI rocky9 and rocky10 by using `fro…

b183b27

…m bin.addheader import`

jfeo added 2 commits October 2, 2026 10:51

Fix lint issues in tests/support/iosupp.py

86501da

jonasbardino reviewed Oct 2, 2026

View reviewed changes

Comment thread .gitattributes

jonasbardino reviewed Oct 2, 2026

View reviewed changes

		TreeDict = Union[dict[str, "TreeDict"], dict[str, str]]


		def write_file(directory: str, name: str, content: str) -> None:



		def add_header(modulename: str, modulebody: str) -> str:
		"""Naive function to add a header, used for generting test data."""

Conversation

jfeo commented Sep 30, 2026

Uh oh!

Uh oh!

jonasbardino left a comment

Choose a reason for hiding this comment

Uh oh!

jfeo commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfeo commented Oct 2, 2026

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!

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!

jonasbardino Oct 2, 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!

jonasbardino left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jfeo commented Oct 2, 2026 •

edited

Loading

jonasbardino Oct 2, 2026 •

edited

Loading