chore: adopt oxfmt for code formatting - #476
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Adopt oxfmt for Code FormattingChore🔧 Introduces oxfmt ( Changes
PR Bot InformationVersion:
|
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The CI lint job has no setup-node step, but oxfmt requires Node ^20.19.0 || >=22.12.0 (as declared in the lockfile). Without an explicit setup, the runner uses whatever Node version ships with ubuntu-latest, which may not satisfy that constraint and would cause format:check to fail with an engine incompatibility error. Consider adding an actions/setup-node step to the lint job pinning a compatible version (e.g. 22.x), matching the pattern already used in the test job. The $schema reference to ./node_modules/oxfmt/configuration_schema.json is the only other noteworthy point — it won't resolve in editors before npm install — but since the $schema is purely advisory for IDE validation and doesn't affect CI, it's a minor inconvenience rather than a defect.
PR Bot Information
Version: 1.29.18
- File Content Strategy: Full file content
- Correlation ID:
a5e7ba40-94b3-11f1-9980-719ea635abfd - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
| @@ -0,0 +1,21 @@ | |||
| { | |||
| "$schema": "./node_modules/oxfmt/configuration_schema.json", | |||
There was a problem hiding this comment.
Best Practices: The $schema path points to ./node_modules/oxfmt/configuration_schema.json, which only resolves after npm install. If the schema file is absent (e.g. fresh clone before install, or in editors that resolve schemas at open time before install), the schema reference silently fails to validate. A remote URL (e.g. from a CDN or the package's GitHub raw URL) would work universally. If the local path is intentional for offline use, this is acceptable but worth noting.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
Add oxfmt (0.63.0) as the code formatter: - .oxfmtrc.jsonc matching house style (single quotes, no semi, width 120, 2-space, no trailing comma, arrowParens: avoid) - format / format:check npm scripts (oxfmt --write / --check) - oxfmt devDependency + lockfile - format:check step in CI lint job No git hook is added; scripts + CI check only. jest.config.js, CHANGELOG.md, node_modules and package-lock.json are excluded via ignorePatterns.
Repo-wide reformat by oxfmt 0.63.0. Whitespace/line-reflow only; no logic changes (verified via git diff -w).
… ConsoleMetricExporter (#479) ## What Consolidates all outbox/metrics test-quality work into one PR (formerly split as #479 + the stacked #480). - **In-memory metric reader** — `test/bookshop/lib/MyInMemoryMetricReader.js`, the metrics counterpart to `MyInMemorySpanExporter` (#465). Mirrors production **DELTA** temporality: SUM counters are accumulated across flushes into per-series running totals; GAUGE datapoints keep the latest absolute value. Wired via the `metrics-outbox`, `metrics-outbox-disabled`, and `metrics` profiles in `.cdsrc.json`. - **Outbox suites off console spying** — the three `metrics-outbox*.test.js` suites drop the `console.dir` spy and fixed `wait()` sleeps in favor of the reader + an `expectEventually()` force-flush polling helper (fails fast if the meter provider isn't wired). Folds in #445's polling approach. - **ConsoleMetricExporter unit test** — new `test/console-metric-exporter.test.js`, a pure unit test of the exporter's formatting (db.pool table, queue table, other single-vs-array, tenant variants, host-metrics aggregation, shutdown→FAILED), mirroring `console-span-exporter.test.js`. - **`metrics.test.js`** converted from scraping `cds.test.log()` output to asserting on the in-memory reader's datapoints. Metrics testing now mirrors the tracing side exactly: a to-console unit test **plus** in-memory-exporter–based integration tests. ## Why Follow-up to #465 (span test infra): eliminate console/log spying in the metrics suite and give `ConsoleMetricExporter` direct unit coverage. ## Review addressed - Bot review triaged: explicit `COUNTER_METRIC_NAMES` dispatch for `isCounter`; real wall-clock debounce in the multitenant test; isolation NOTE on the module-level singletons. - Dropped the unused debug-log silencer in the multitenant suite (never asserted). Kept the single-tenant `debugLog` mock — it backs a real `unknown service` assertion. Test-only change (no `lib/` change), so no CHANGELOG entry — consistent with #465/#474/#476. closes #478 Supersedes #445 and #480 (both folded in here) — I'll close them once this merges.
1 participant
Supersedes the #438 oxfmt spike — adopts oxfmt (
0.63.0, pinned) properly.Config (
.oxfmtrc.jsonc)Based on the #438 spike, verified against the
lib/*.jshouse style:singleQuote,semi: false,printWidth: 120,tabWidth: 2,trailingComma: none,arrowParens: avoid.ignorePatternsexcludes*.md,node_modules,package-lock.json,CHANGELOG.md, andjest.config.js(see coordination).Scripts
format→npx oxfmt(write is oxfmt's default)format:check→npx oxfmt --checkNo git hook / husky / lint-staged — CI
format:check+ scripts only (the intrusive hook from the #438 spike is intentionally not carried over).Commits (reviewable split)
chore: add oxfmt formatter tooling— config + scripts + devDep + lockfile + CI stepchore: apply oxfmt formatting— repo-wide reformat (11 files, line-wrapping only; verified non-semantic viagit diff -w)eslint coexistence
@sap/cds/eslint.config.mjsisrecommended+no-unused-vars/no-consoleonly (no stylistic rules) → no conflict.npm run lintstays green.CI
One line added to the
lintjob inci.yml:npm run format:check.Verified
npm run format:check✅ (56 files) ·npm run lint(--max-warnings=0) ✅ ·npm run test→ 53 pass / 14 skip, exit 0 · lockfile resolved from public npm (0 internal-registry URLs).Coordination
Parallel PR #474 (jest→vitest) deletes
jest.config.js— this PR excludes it from formatting (0-line diff confirmed) so no collision.package.jsonchange here is additive (scripts + devDep); lockfile will conflict with #474 — whichever merges second rebases.