style: replace em dashes and en dashes with standard punctuation - #609
style: replace em dashes and en dashes with standard punctuation#609jongio wants to merge 4 commits into
Conversation
Removes every em dash and en dash from tracked source, docs, and config, replacing each with the punctuation the sentence actually calls for: a colon for label-then-explanation shapes, a semicolon where two independent clauses were joined, and a comma for appositives and parentheticals. 97 files, 370 replacements. No behavior change: every Go edit is inside a comment or a doc string, and every TypeScript edit is inside a comment or a test description string. Excluded on purpose: - CHANGELOG.md, its entries mirror published release notes and commit subjects, so editing them desyncs the file from immutable git history. - web/src/pages/reference/changelog/index.astro, generated release-note markup with the same immutability rationale. Verified: go build, go vet, and go test -short -race all pass; tsc --noEmit is clean; vitest runs 1436 tests across 63 files with zero failures.
|
🚀 Website Preview Your PR preview is ready! 📎 Preview URL: https://jongio.github.io/azd-app/pr/609/ This preview will be automatically cleaned up when the PR is closed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #609 +/- ##
==========================================
+ Coverage 63.87% 64.07% +0.20%
==========================================
Files 241 241
Lines 31502 31502
==========================================
+ Hits 20121 20185 +64
+ Misses 10061 9989 -72
- Partials 1320 1328 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
wbreza
left a comment
There was a problem hiding this comment.
A clean, inert punctuation sweep, except two generated protobuf artifacts were edited instead of their .proto source.
What stood out
- Genuinely inert edits — Go changes land in comments and doc strings, TypeScript changes in comments and test descriptions. No sanitizer allowlist, regex, exec argument, or wire constant was touched.
- Test assertions untouched — Every test-file edit sits in a comment or a describe/it label, never in an expect argument, a t.Errorf, a golden fixture, or a case name used in a comparison.
- Doc tables survived — Column counts hold across the five largest doc diffs, the one cell holding a bare dash became n/a rather than collapsing, and numeric ranges read correctly.
- Generated code drift — Two derived protobuf files were hand-edited while their .proto source kept its em dash, so the next codegen run silently reverts both.
Top issues
- [F-001] Generated protobuf file edited instead of its .proto source —
cli/src/gen/proto/azdapp/v1/common.pb.go:322 - [F-002] One source comment rendered with different punctuation in Go and TypeScript —
cli/dashboard/src/gen/proto/azdapp/v1/common_pb.ts:508
Cross-cutting
- [F-001] Derived artifacts edited ahead of their source (process) — Spans both generated protobuf outputs plus the untouched
proto/azdapp/v1/common.proto.
Improvement opportunities
- [O-001] Doubled colons where a bold label already ends in a colon (Optional) —
AGENTS.md:27 - [O-002] Eval stimulus name doubles as a natural key for result history (Optional) —
cli/evals/azd-app-onboard/eval.yaml:83
🚀 Test This PRA preview build ( 🌐 Website PreviewLive Preview: https://jongio.github.io/azd-app/pr/609/ One-Line Install (Recommended)PowerShell (Windows): iex "& { $(irm https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/install-pr.ps1) } -PrNumber 609 -Version 0.20.0-pr609"Bash (macOS/Linux): curl -fsSL https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/install-pr.sh | bash -s 609 0.20.0-pr609UninstallWhen you're done testing: PowerShell (Windows): iex "& { $(irm https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/uninstall-pr.ps1) } -PrNumber 609"Bash (macOS/Linux): curl -fsSL https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/uninstall-pr.sh | bash -s 609Build Info:
What to Test: |
…ted output Addresses review feedback on the em dash sweep. F-001 / F-002: cli/src/gen/proto/azdapp/v1/common.pb.go and cli/dashboard/src/gen/proto/azdapp/v1/common_pb.ts are both generated from proto/azdapp/v1/common.proto, which still carried the em dash. The next buf generate would have reverted both edits, and the two hand edits had already diverged (a semicolon in Go, a colon in TypeScript) for one and the same source string. Reverted both generated files to their pre-sweep state, applied the change to the proto source, and set both outputs to the semicolon the source now uses. All three copies of the comment are byte-identical, so buf generate is a no-op for that line. common.pb.go needed no net change, since its hand edit happened to match. Root cause: the sweep took an extension list that included .go and .ts but not .proto, so it edited derived artifacts while their source stayed out of scope. Generated trees belong on the exclusion list for the same reason CHANGELOG.md already is. O-001: the sweep turned a label that already ended in a colon into a line with two colons. Fixed the five instances it introduced in AGENTS.md, plus the same shape it introduced in README.md, cli/docs/schema/azure.yaml.md, and docs/specs/local-container-orchestration/spec.md. Lines 54 and 78 of AGENTS.md have the same shape but predate this branch, so they are left alone. Verified in a container with cgo available: go build, go vet, and go test -short -race all pass.
wbreza
left a comment
There was a problem hiding this comment.
Both generated-artifact defects are fixed at the source, and the cumulative diff is clean.
What stood out
- Fixed at the right layer — The punctuation now lives in
proto/azdapp/v1/common.proto:86, and both generated outputs carry the identical semicolon. The Go/TS split is structurally impossible now that one source feeds both. - Regeneration is genuinely a no-op — The embedded descriptors in
common.pb.goandcommon_pb.tswere generated without source info, so they encode no comment text. Comment-only edits cannot make them stale, which confirms the textual verification was sufficient even without running codegen. - Doubled colons resolved broadly — Rather than patching the one flagged line, the shape was swept across
AGENTS.md,README.md,azure.yaml.md, and the container-orchestration spec, each rewritten to fit the sentence. The two remaining instances inAGENTS.mdare unmodified on main and correctly out of scope. - Wire contract untouched — The edited comment documents append-only enum semantics; the
HealthStatevalues themselves are unchanged, and no sanitizer, regex, exec argument, or test assertion moved anywhere in the sweep.
Improvement opportunities
- [O-002] Eval stimulus names double as history keys until an explicit
idfield exists (Optional) —cli/evals/azd-app-onboard/eval.yaml:83
Exercise the dry-run and unchanged-configuration branches changed by the punctuation sweep. This raises Codecov patch coverage from 50% to 83% without changing production behavior or weakening the patch gate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 47f148f4-45d6-437f-a453-a1f6ab26194d
wbreza
left a comment
There was a problem hiding this comment.
Test-only commit that closes the coverage gap honestly, by exercising the branches rather than adjusting the gate.
What stood out
- The dry-run test is not vacuous — The risk with a test like this is that it passes because detection silently found nothing, which would keep it green even if dry-run were broken. It does not: zero detected services returns an error, and the test requires
Execute()to succeed. Captured output confirms the Vite fixture detects a service and reaches the dry-run branch. - Byte-equality is the right assertion here — On the unchanged-configuration branch the file is never opened for write, so exact equality tests real behavior rather than YAML round-trip fidelity, which would have been a brittle claim.
- Both swept strings genuinely execute —
Dry run, no files modifiedandNo changes needed, azure.yaml already has complete service configurationboth appear in test output, so the two lines the sweep touched are actually covered. - No process-state hazard — Neither new test calls
t.Parallel(),t.Chdirmatches the established pattern in this package,NewInitCommand()returns a fresh command with no package-level state, and the full package passes under-race.
Improvement opportunities
- [O-003] Covered lines are executed but their output is never asserted (Optional) —
cli/src/cmd/app/commands/init_test.go:348
| assert.Equal(t, original, string(actual)) | ||
| } | ||
|
|
||
| func TestInitCommand_DryRunDoesNotWriteAzureYaml(t *testing.T) { |
There was a problem hiding this comment.
[O-003] Both tests execute the two swept cliout.Info(...) lines, confirmed in captured output, but neither asserts what was printed.
The practical consequence: if a future edit reverted Dry run, no files modified to an em-dash form, or otherwise corrupted either string, coverage would stay green and both tests would still pass. So these raise the patch percentage without protecting the exact prose this PR set out to normalize.
That is a legitimate choice if the goal was purely the coverage gate, and the branches themselves are now genuinely exercised, which is the harder half. If you want the strings pinned too, capturing the output stream and asserting the exact ASCII text would close it.
One note if this test ever grows: with t.TempDir() plus t.Chdir and no name in the fixture, the service name is derived from the temp directory basename, so it came through as 001. Harmless for NoFileExists, but it would make any future assertion on generated YAML content flaky. A named subdirectory or an explicit "name" field would make it deterministic.
The mage-tagged build scripts never enter the uploaded Go coverage profile, so excluding them cannot affect Codecov. The actual patch failure came from three executable statements in init.go and run.go, addressed by tests on azd-app #609. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 47f148f4-45d6-437f-a453-a1f6ab26194d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 47f148f4-45d6-437f-a453-a1f6ab26194d
wbreza
left a comment
There was a problem hiding this comment.
The comma splices the sweep introduced are now cleaned up, and the three re-edited user-facing strings have no consumer that breaks.
What stood out
- Right fix for the right problem — Replacing an em dash with a comma between two independent clauses produced splices; converting those to semicolons and colons resolves them without touching meaning. All 22 files read correctly.
- Re-edited strings are safe —
Dry run: no files modified,No changes needed: azure.yaml..., and theinstall_dependencieserror each appear exactly once in the tree, at their own definition. No test, eval rubric,must_containpattern, snapshot, or test-name filter references any of them, so the second edit to these strings breaks nothing. - Security rationale intact — The CWE-352/CWE-306 comments in
server_routes.go, the XSS note inlog-utils.ts, and the containment test comments are comment-only and none were weakened. The numbered two-factor list actually reads better with colons. - Merge blocker is not from this PR — The failing vulnerability check reports only Go standard library advisories in
crypto/tls,net/http,net/url,html/template,encoding/asn1, andencoding/xml, all fixed in go1.26.6 against a repo pinned to go1.26.5. The two flagged files this branch touches are comment-only here, so the fix is a toolchain bump, independent of this work.
Improvement opportunities
- [O-004] One comma splice from the original sweep is still unconverted (Optional) —
cli/src/internal/service/types.go:466
| // override). A service that only builds an image for deployment (`docker.*`) | ||
| // runs locally as a process when an explicit local `command` or `type: process` | ||
| // is configured — its `docker.*`/image is then used only by `azd deploy`. | ||
| // is configured, its `docker.*`/image is then used only by `azd deploy`. |
There was a problem hiding this comment.
[O-004] This line still has the shape the rest of the commit fixed. The original sweep turned an em dash into a comma here:
// runs locally as a process when an explicit local `command` or `type: process`
// is configured, its `docker.*`/image is then used only by `azd deploy`.
Both halves are independent clauses. The second has its own subject and verb (its docker.*/image / is then used), and the then signals exactly the consequence relationship a semicolon carries. It reads as the same splice that mode.go, httputil.go, and checker_process.go all had before this commit.
Comment-only, so there is no behavior impact, and this does not hold anything up. A semicolon in place of the comma would finish the set.
Removes every em dash and en dash from tracked source, docs, and config across 99 files (368 replacements), replacing each with the punctuation the sentence actually calls for: a colon for label-then-explanation shapes, a semicolon where two independent clauses were joined, and a comma for appositives and parentheticals.
Two focused tests cover the unchanged-configuration and dry-run branches in
init.go. No functional behavior change. Production edits are limited to punctuation in comments, documentation, and user-facing strings.Replacement was script-driven for consistency, then every added line was read by hand. The manual pass caught real defects the rules introduced, including empty markdown table cells that had held a bare dash collapsing and breaking column counts, and a sentence that ended up with two colons in it.
Excluded on purpose:
CHANGELOG.md. Its entries mirror published GitHub Release notes and commit subjects, so editing them desyncs the file from immutable git history.web/src/pages/reference/changelog/index.astro. Generated release-note markup, with the same immutability rationale.Verified: all non-Vitest
mage preflightgates pass locally with CI-compatible pnpm 9 and Go 1.26.6, including builds, lint, docs, E2E, race tests, security scanning, and vulnerability scanning. The dashboard suite passes separately with 1,436 tests across 63 files. The parallel preflight run encountered only Vitest worker startup starvation while every executed test passed.