format: normalize quote style across single, double, and backtick strings - #164
Closed
claude[bot] wants to merge 4 commits into
Closed
format: normalize quote style across single, double, and backtick strings#164claude[bot] wants to merge 4 commits into
claude[bot] wants to merge 4 commits into
Conversation
claude
Bot
changed the title
Convert string literals to a preferred quote style: double quotes by default, single quotes when the content contains a double quote, and backticks when it contains both a double and a single quote. Applies to rval strings (via the quoted_string choke point) and promisers.
claude
Bot
force-pushed
the
format-single-to-double-quotes
branch
from
June 24, 2026 18:49
c6a5d1b to
e2d6aa2
Compare
olehermanse
requested changes
Jun 24, 2026
Comment on lines
+101
to
+102
| Backtick strings are taken literally - CFEngine processes no escapes | ||
| inside them. Single- and double-quoted strings recognize only the escapes |
Member
There was a problem hiding this comment.
This is not true, backtick strings are treated the same, and escapes are processed.
Comment on lines
+119
to
+121
| if nxt == "\n": # line continuation: drop both characters | ||
| i += 2 | ||
| continue |
Member
There was a problem hiding this comment.
This behavior was not requested and is not covered by the test.
Comment on lines
+113
to
+118
| if c == "\\" and i + 1 < len(inner): | ||
| nxt = inner[i + 1] | ||
| if nxt in ("\\", '"', "'"): | ||
| out.append(nxt) | ||
| i += 2 | ||
| continue |
Member
There was a problem hiding this comment.
The formatting test does not have \\ (or \\\\), should cover that too.
Comment on lines
+143
to
+148
| if ( | ||
| len(literal) < 2 | ||
| or literal[0] != literal[-1] | ||
| or literal[0] not in ("'", '"', "`") | ||
| ): | ||
| return literal |
Member
There was a problem hiding this comment.
Should this be an assert? I don't see when this would be the case?
Comment on lines
+159
to
+162
| if target == "`" and "`" in content: | ||
| # Needs all three quote styles at once; a backtick string cannot | ||
| # contain a backtick, so leave the literal as written. | ||
| return literal |
Member
There was a problem hiding this comment.
This is also not covered in the test.
Address review on PR #164: - Backtick strings process the SAME escapes as single/double quotes (\\, \", \'); a backtick string is not literal. Its only special property is that the delimiter itself cannot be escaped, so a literal backtick can never appear inside one. Unified _decode_literal (drops its delim arg) and _encode_literal (now escapes backslashes for backticks too) accordingly. - Removed the unrequested backslash-newline line-continuation handling from _decode_literal. - Turned the literal-shape guard in _normalize_quotes into an assert, since callers always pass a real quoted_string literal. - Expanded the 012_quotes golden fixture with backslash cases (l/m/n/p) and an all-three-quotes case (o) to cover the unified escape behavior and the backtick encode branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U4hEZuqiuEFH2zy8wWGwyD
olehermanse
requested changes
Jun 24, 2026
Replaced the _decode_literal/_encode_literal helpers with a single-pass, escape-aware re-quote (_scan_quotes + _requote) so converting between quote styles passes through escaped backslashes, line continuations, and other backslash sequences unchanged, and only re-escapes the target delimiter quote character. The old decode/encode round-trip would mangle a single-quoted line-continuation string into a doubled backslash. Fixed fixture case q to convert a single-quoted line-continuation string to double quotes with the continuation preserved verbatim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U4hEZuqiuEFH2zy8wWGwyD
nickanderson
approved these changes
Jul 22, 2026
nickanderson
left a comment
nickanderson
left a comment
Member
There was a problem hiding this comment.
I agree with the suggested expectations I just think this should require opting in, not done by default with format. Something link could surface. But then I or someone else will also ask for a way to suppress the warning with some kind of comment.
Contributor
|
Marking this PR as stale due to inactivity; it will be closed in 7 days. |
olehermanse
marked this pull request as ready for review
August 24, 2026 17:30
Member
|
I will postpone this a bit. I think it should be done by default in
However I believe we have quite a few tests which test different types of quotes, and in those cases we do want to format them, but not change the quotes. So we'll have to figure out what to do there. Closing for now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Ole Herman · Slack thread
Before / After
cfengine formatnow normalizes string-literal quote style across double, single, and backtick quotes. It prefers double quotes; falls back to single quotes when the string contains a double-quote character; and uses backticks only when the string contains both a double and a single quote.Examples:
'hello'becomes"hello"`hello`becomes"hello"'say "hi"'stays single-quoted (it contains a double quote but no single quote)`say "hi"`becomes'say "hi"''he said "hi" it\'s') becomes the backtick form`he said "hi" it's`If a string somehow needs all three styles at once (it contains a double quote, a single quote, and a backtick), it is left unchanged, since no single style can hold it without escaping.
How
A
_normalize_quotes/_decode_literal/_encode_literaltrio insrc/cfengine_cli/format.pydecodes a literal to its logical content (honoring CFEngine's escaping rules — single/double quotes recognize only\\,\",\', and a backslash-newline line continuation; backticks process no escapes), chooses the preferred style from that content, and re-encodes. It is applied at thequoted_stringchoke point (stringify_single_line_node) and the promiser site (_promiser_text), covered by thetests/format/012_quotesgolden fixture.Note for review
Promisers are normalized too (e.g.
reports: 'hello';becomesreports: "hello";). Calling this out explicitly so it can be vetoed if promisers should be left alone.Generated by Claude Code