cfengine format: Refactored code, added type hints and docstrings, expanded tests, and fixed small issues by olehermanse · Pull Request #55 · cfengine/cfengine-cli

olehermanse · 2026-04-09T20:11:36Z

No description provided.

cf-bottom · 2026-04-13T13:03:57Z

Thanks for submitting a pull request! Maybe @craigcomstock can review this?

src/cfengine_cli/format.py

craigcomstock · 2026-04-13T14:53:25Z

src/cfengine_cli/format.py

+
+
+def _has_trailing_comma(middle):
+    """Check if list middle nodes end with a trailing comma."""


middle? I know this is just a function parameter name but seems like it should be more like just list_of_nodes or something like that.

With this tree-sitter code you often have to consider what the list of nodes looks like - it has commas and curly braces as separate elements.

The reason for the name is that you have a list of nodes like;

nodes = ["{", "a", ",", "b", "}"]

So nodes[1:-1] are referred to as middle (what's left after removing the ends on each side).

This is the kind of things that I would like to see in the doc string. I.e., the parameter middle contains tokens between the two curly braces of a list.

@larsewi 08d626b

craigcomstock · 2026-04-13T16:00:19Z

src/cfengine_cli/format.py

+        return True
+
+    # Multi-line with split stakeholder
+    if _has_stakeholder(children) and _stakeholder_needs_splitting(


Shouldn't the list of stakeholders be reformatted for trailing comma even if the stakeholder has no comments which is the condition included invisibly by _stakeholder_needs_splitting() as I read it.

My example I was trying was

bundle agent main { reports: "hi" -> { "yoda", "boba", }; }

And was surprised to not see some of this code exercised.

This is a bug I can fix. Adding / removing commas is a bit more "tricky" than the other reformatting we do, since commas are nodes in the syntax tree, and since comment nodes can occur pretty much everywhere.

Tests: 63348d2

Fix: 88ff48d

craigcomstock · 2026-04-13T16:05:59Z

src/cfengine_cli/format.py

+    """Format the middle elements (between { and }) of a stakeholder list."""
+    if not any(n.type == "comment" for n in middle):
+        if _has_trailing_comma(middle):
            return split_generic_list(middle, indent, line_length)


I don't see these functions properly doing what they say: always add trailing comma in multi-line lists:

191 # Always add a trailing comma on multi-line lists 192 if elements and not elements[-1].endswith(","): 193 elements[-1] = elements[-1] + ","

An example:

bundle agent main { reports: "hi" -> { "yoda", # this is yoda of course "boba" # and yet another star wars character }; }

Is formatted as:

bundle agent main { reports: "hi" -> { "yoda", # this is yoda of course "boba" # and yet another star wars character }; }

And it would seem according to the comment in the code I mention above that "boba" should be followed by a comma.

Tests: 63348d2

Fix: 88ff48d

craigcomstock · 2026-04-13T16:32:44Z

src/cfengine_cli/format.py

        return False
-    prefix = promiser_line(children)
-    if not prefix:
+    line = _promiser_line_with_stakeholder(children)


Do we have code coverage maybe? I am not sure when this case would be reached. Seems like most cases would pop out above at line 361:

if _has_stakeholder(children) and _stakeholder_needs_splitting(

I can add comments to make it as clear as possible.

src/cfengine_cli/format.py

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

Other PRs were merged to master, and so there are test failures / linting errors I had to fix now. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

JSON code was returning 1 (error) for case where file was reformatted. Also ended up refactoring a bit to make this as clear as possible. Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

…ilining Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

larsewi

All in all, a great improvement

larsewi · 2026-04-14T12:46:53Z

tests/unit/test_format.py

+    if root.type == node_type:
+        return root
+    for child in root.children:
+        found = _find_opt(child, node_type)
+        if found:
+            return found


Is it not sufficient to just call the _find_opt() and check the returned value?

larsewi · 2026-04-14T12:48:37Z

tests/unit/test_format.py

+    raise ValueError(f"No node of type {node_type!r} found")
+
+
+def _find_opt(root: Node, node_type: str):


Maybe there is a better name that can be used here? E.g., _find_or_none

larsewi · 2026-04-14T12:55:55Z

tests/unit/test_format.py

+def test_maybe_split_rval_too_long():
+    root = _parse('bundle agent x { vars: "v" slist => { "a", "b" }; }')
+    list_node = _find(root, "list")
+    result = maybe_split_rval(list_node, 6, 999, 80)


It would be nice if you'd suffix the argument name, so that it is clear what these numbers are.

larsewi · 2026-04-14T13:09:41Z

I would just merge and summit follow-up PRs if you want to change anything more now.

olehermanse force-pushed the continuation branch from 74de2df to b829661 Compare April 9, 2026 20:12

olehermanse changed the title ~~WIP~~ WIP formatting improvements Apr 9, 2026

olehermanse force-pushed the continuation branch 4 times, most recently from 04f4df3 to 4f76e8b Compare April 13, 2026 12:58

olehermanse marked this pull request as ready for review April 13, 2026 13:00

olehermanse changed the title ~~WIP formatting improvements~~ cfengine format: Refactored code, added type hints and docstrings, and expanded tests Apr 13, 2026

olehermanse changed the title ~~cfengine format: Refactored code, added type hints and docstrings, and expanded tests~~ cfengine format: Refactored code, added type hints and docstrings, expanded tests, and fixed small issues Apr 13, 2026

olehermanse force-pushed the continuation branch from 634ef92 to b8a080d Compare April 13, 2026 13:22

olehermanse requested review from larsewi and victormlg and removed request for larsewi and victormlg April 13, 2026 13:23

olehermanse force-pushed the continuation branch from 18d010a to be21cdc Compare April 13, 2026 13:30

olehermanse requested review from SimonThalvorsen, craigcomstock, larsewi and victormlg April 13, 2026 14:13

olehermanse force-pushed the continuation branch from d5e3685 to be21cdc Compare April 13, 2026 14:33

craigcomstock reviewed Apr 13, 2026

View reviewed changes

olehermanse and others added 8 commits April 13, 2026 18:54

format.py: Refactored using Claude Code

214a218

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

format.py: Added docstrings and type hints

d744105

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

tests: Expanded formatting unit tests

3f34f17

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

Fixed issues which came after merge

f45f364

Other PRs were merged to master, and so there are test failures / linting errors I had to fix now. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole.elgesem@northern.tech>

Fixed inconsistency between format json and format policy function

09c8664

JSON code was returning 1 (error) for case where file was reformatted. Also ended up refactoring a bit to make this as clear as possible. Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

cfengine format: Made sure trailing commas are always added when mult…

6ed066f

…ilining Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

Adjusted formatting tests with more trailing comma cases

63348d2

Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

Fixed trailing comma and closing curly brace issues

88ff48d

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

olehermanse force-pushed the continuation branch from be21cdc to 88ff48d Compare April 13, 2026 17:34

olehermanse added 2 commits April 13, 2026 20:09

Improved can_single_line_promise() with comments and asserts

804e204

Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

format.py: Improved readability of _has_trailing_comma()

08d626b

Signed-off-by: Ole Herman Schumacher Elgesem <ole@northern.tech>

larsewi approved these changes Apr 14, 2026

View reviewed changes

olehermanse merged commit 6e9a4f2 into cfengine:main Apr 14, 2026
5 checks passed

olehermanse deleted the continuation branch April 14, 2026 14:14



		def _has_trailing_comma(middle):
		"""Check if list middle nodes end with a trailing comma."""

		raise ValueError(f"No node of type {node_type!r} found")


		def _find_opt(root: Node, node_type: str):

Conversation

olehermanse commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cf-bottom commented Apr 13, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olehermanse Apr 13, 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!

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!

larsewi left a comment • 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!

larsewi commented Apr 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

olehermanse commented Apr 9, 2026 •

edited

Loading

olehermanse Apr 13, 2026 •

edited

Loading

larsewi left a comment •

edited

Loading