Align prettier wrappers with project-root CWD and drop path rewriting - #56
Align prettier wrappers with project-root CWD and drop path rewriting#56UltraBob wants to merge 2 commits into
Conversation
The prettier check wrapper was the only command in the suite that ran from the Drupal docroot, and the pair compensated by rewriting user-supplied relative paths in opposite directions (check stripped the docroot prefix, fix added it). A relative path therefore meant something different to ddev prettier than to every other wrapper, and ddev prettier accepted a path shape stock prettier would reject. Both wrappers now run from the project root, pass user-supplied relative paths through untouched, translate host-absolute paths via map_path only, and inject the recorded docroot as the default target on bare runs - the same shape as the stylelint pair. The docroot CWD was not needed for CI parity: config and ignore files are passed as absolute paths, and Prettier resolves ignore patterns relative to the ignore file's location. Arguments are now split into options and file targets, and the targets are passed after --, so a filename that begins with a dash is treated as a path rather than an unknown option. Behavior changes beyond the CWD alignment: - -c is no longer treated as --config. In prettier's CLI, -c is short for --check; it now passes through. Use --config to point at a config file. - prettier-fix now scopes by the project .prettierignore alone (matching the check wrapper) instead of prettier's default .gitignore + .prettierignore set, so check and fix agree on the file set. Files that are gitignored but not prettierignored are now formatted by fix, since check already flagged them. - --stdin-filepath suppresses the default docroot target so piped stdin is actually checked, and its value is host-path mapped. - Both wrappers resolve prettier/bin/prettier.cjs and invoke it via node, like the rest of the suite, instead of the check wrapper depending on the .bin shim's executable bit. - The injected default docroot is validated to stay inside the project root, and a value-taking flag left dangling at the end of the arguments no longer swallows the injected target. - ddev prettier rejects --preview with a pointer to prettier-fix instead of exiting 0 without checking anything. Preview mode is rebuilt so it cannot disagree with the apply it precedes. It previously copied the project tree, formatted the copy, and diffed the copy against the real tree. Every difference between the copy and the real project - excluded directories, config and ignore files resolved against the wrong root, concurrent writes - showed up as a preview that promised something other than what apply did. It now asks prettier which files the apply run would rewrite, using the identical arguments that run will use, then diffs each of those files against prettier's own formatted output. There is no copy to diverge from the project, so the patch is exactly the pending edit: - Custom --config and --ignore-path values, and ignore patterns anchored to a directory, behave the same in the preview as in the apply. - Files in directories the old copy excluded are previewed correctly. - The patch is written with project-relative a/ b/ headers, quoted the way git quotes names that contain whitespace, so it applies with patch -p1. - --preview is rejected alongside --stdin-filepath and explicit output-mode flags, which would have made the listing run write files. - The patch is truncated before work starts, so a failed run cannot leave an earlier run's patch behind to be mistaken for the current one. - Report directory, temp directory, and diff failures abort cleanly. - A file literally named --preview can be targeted after --.
stylelint-fix dropped --preview in the wrapper refactor and points users at git diff instead, and the preview flow reports a change count and patch location rather than displaying the patch. Update the fix-commands paragraph and the dcq-reports note to match.
Needs a deep review before mergingFlagging this explicitly: do not merge on the strength of the green checks. This branch went through four full-scope review rounds, and every round found real defects — including defects inside the previous round's fixes. That pattern is the reason it is a draft. What each round surfaced, in order:
Rather than patch round 4's findings individually, the preview was rebuilt to remove the failure class: no tree copy, so nothing can diverge from the real project. That redesign is newer than any review round, so it has had no adversarial review at all — only the direct verification described in the PR body. It is the part that most needs eyes. Specific things worth a reviewer's attention:
The eslint pair still uses the copy-based preview architecture that was replaced here, so the round 2–4 findings are worth re-checking against it as separate issues. |
Test status, stated preciselyThe bats suite for these commits passed in CI ( Local runs need a caveat. The full local suite passed twice against earlier revisions of this branch, and the interactive Everything else in the PR body was checked by hand with real |
Local test caveat withdrawn — both suites now pass against this revisionThe local Docker daemon recovered, so I re-ran the suites serially against the final commits:
That closes the gap described in my previous comment: the earlier failure to complete was the local parallel-run race, not the code. Together with The request for deep review stands unchanged — the preview redesign is still newer than every adversarial review round, and passing tests is not the same as having been reviewed. |
1 participant
Closes #46.
What changed
commands/web/prettierwas the only wrapper in the suite that ran from the Drupal docroot. To compensate, the pair rewrote user-supplied relative paths in opposite directions — the check wrapper stripped theweb/prefix, the fix wrapper added it. A relative path therefore meant something different toddev prettierthan toddev phpcs, andddev prettieraccepted a path shape stock prettier rejects.Both wrappers now run from the project root, pass relative paths through untouched, translate host-absolute paths via
map_pathonly, and inject the recorded docroot as the default target on bare runs — the same shape as the stylelint pair (precedent: #32).The "check this first" question in the issue is answered: the docroot CWD was not load-bearing. Config and ignore files are passed as absolute paths, and Prettier resolves ignore patterns relative to the ignore file's own directory, so the CWD only affected relative-argument resolution and the bare-run target.
Behavior changes worth a release note
-cis no longer treated as--config. In prettier's CLI-cis short for--check; it now passes through. Use--configto point at a config file. The old grouping silently swallowed the next argument.prettier-fixnow scopes by the project.prettierignorealone, matching the check wrapper, instead of prettier's default.gitignore+.prettierignoreset. Files that are gitignored but not prettierignored are now formatted by fix — previously check flagged them and fix refused to touch them.--, so a filename beginning with a dash is treated as a path rather than an unknown option.--stdin-filepathnow suppresses the default docroot target, so piped stdin is actually checked instead of silently ignored while the whole docroot got scanned.ddev prettier --previewnow errors with a pointer toprettier-fixinstead of exiting 0 without checking anything.Preview mode was rebuilt
prettier-fix --previewpreviously copied the project tree, formatted the copy, and diffed the copy against the real tree. Every way the copy differed from the real project — excluded directories, config and ignore files resolved against the wrong root, concurrent writes — produced a preview that promised something other than what apply did. Successive review rounds kept finding new instances of that same class.It now asks prettier which files the apply run would rewrite, using the identical arguments that run will use, then diffs each file against prettier's own formatted output. There is no copy, so there is nothing to diverge:
--config/--ignore-pathvalues and directory-anchored ignore patterns behave identically in preview and apply..trunk,dcq-reports, nestedvendor) are previewed correctly.a/b/headers, quoted the way git quotes names containing whitespace, so it applies withpatch -p1.--previewis rejected alongside--stdin-filepathand explicit output-mode flags, which would have made the listing run write files.Testing
Verified with real
ddevcommands againstsandboxes/runway-ops(fresh add-on install with root Node toolchain) andux-test-d11: bare runs, root-relative / docroot-relative / host-absolute paths,.incexclusion,--help,--version,--, both--configforms, host shim invocation,ddev checksintegration, preview→apply parity with a custom ignore file, filenames containing spaces, leading-dash filenames, a hostile.dcq-docroot, and the--previewflag-conflict guards.bash -npasses on both wrappers.Docs
README's fix-commands paragraph and
dcq-reports/note corrected —stylelint-fixdropped--previewin its refactor, and the preview flow reports a change count and patch path rather than displaying the patch.Follow-ups not in this PR
ddev eslint --previewhas no friendly rejection..ddev/**is not in the shipped.prettierignore, so whole-root scans trip over the add-on's own#ddev-generatedassets.