Repository navigation
feat(cli): generate the CLI reference tables in cli/README.md from the option definitions - #2419
mbiernacik wants to merge 6 commits into
Conversation
|
Since #2418 modifies both the CLI and README.md, one of these PRs must be adjusted to align with the other. |
| name: windows-smoke-test | ||
| path: bazel-bin/cli/tests/windows/smoke_test_bundle.js | ||
|
|
||
| # Reuses the //cli build above. Kept last so that a failure doesn't block the Windows |
There was a problem hiding this comment.
nit: remove traliing spaces
There was a problem hiding this comment.
The step is gone now (the README tests run in Cloud Build again, see the run_tests_on_cloudbuild thread), so test.yaml is back to its state on main.
| ./scripts/run_tests | ||
| # Run unit tests. The cli/README.md tests (tag cli_docs) are skipped here: they run on GitHub | ||
| # Actions (build-linux job in .github/workflows/test.yaml) for every PR, including forks. | ||
| ./scripts/run_tests --test_tag_filters=-integration,-cli_docs |
There was a problem hiding this comment.
Excluding -cli_docs here removes readme_tests from test-pull-request-trigger, which is the required branch protection check on main (whereas build-linux in .github/workflows/test.yaml is not a required check). Since readme_tests takes around 100ms, please keep it enabled in scripts/run_tests_on_cloudbuild so stale READMEs block merging out of the box without changing repo branch protection settings.
There was a problem hiding this comment.
Good point, I missed that test-pull-request-trigger is the required check. Reverted:
- the README tests run in Cloud Build again (
scripts/run_tests_on_cloudbuildis back to its state onmain), - the GitHub Actions step and the
cli_docstag are removed, contributing.mdnow points to the unit tests instead of thebuild-linuxjob.
So there's a single place for this check, and it blocks merging without any branch protection changes.
| name: windows-smoke-test | ||
| path: bazel-bin/cli/tests/windows/smoke_test_bundle.js | ||
|
|
||
| # Reuses the //cli build above. Kept last so that a failure doesn't block the Windows |
There was a problem hiding this comment.
The comment states Kept last so that a failure doesn't block the Windows smoke tests, which is inaccurate: test-windows declares needs: build-linux, so if the last step of build-linux fails, GitHub Actions skips test-windows altogether. Either move the README check into its own job/step after test-windows, or update the comment.
There was a problem hiding this comment.
You're right, with needs: build-linux a failure in that step would skip test-windows. The step is removed now, so this no longer applies.
| // first (so that the escapes added below can't be altered), then characters that markdown or GFM | ||
| // tables would interpret (`|` ends the cell, `*` and `` ` `` format text), then HTML (e.g. the | ||
| // "<job-prefix>" in "dataform-<job-prefix>-" would otherwise be hidden as an unknown tag). | ||
| function textCell(text: string): string { |
There was a problem hiding this comment.
textCell escapes |, * and ` so plain-text option descriptions are not interpreted as Markdown, but it does not escape _ or [ and ] (which Markdown interprets as italics text or links text). Consider including _, [ and ] in the escaped character set in textCell.
I also wonder if there is out of the shelf lib that could help us here. Before adding one - please conversate.
There was a problem hiding this comment.
Done. textCell now also escapes [, ] and ~ (GFM strikethrough), plus _ at word boundaries. An intraword _ (e.g. workflow_settings.yaml) can't start or end emphasis in CommonMark/GFM, so it's left as is to keep the README source readable. URLs are left as is too, since a backslash would break GitHub's autolinking. Tests cover all of these.
On a library: I'd rather not add a runtime dependency for this:
- escaping-only packages (e.g.
markdown-escape) are a few lines and wouldn't add much, mdast-util-to-markdownpulls in the unified ecosystem for a single function,markdown-itis only in the lockfile as a transitive dependency of jsdoc.
What might be worth it is markdown-it as a devDependency for a round-trip test: render the generated tables and check that each cell's text equals the option's describe. That would guarantee the escaping instead of relying on a character list. WDYT? Happy to do it in this PR or as a follow-up.
|
|
||
| export const emitLineageOption: INamedOption<yargs.Options, IRunArgs> = { | ||
| name: "emit-lineage", | ||
| option: { |
There was a problem hiding this comment.
In emitLineageOption, the parenthetical (requires datalineage.googleapis.com enabled and roles/datalineage.producer, see docs/lineage.md) prints a repo-relative path in dataform help run and duplicates the Notes bullet in cli/README.md (consider moving the prerequisites to Notes like in compile and format). In jitTimeoutOption, consider replacing the internal proto field name jitCode with user-facing wording like Each JiT action.
There was a problem hiding this comment.
Done:
--emit-lineage: the prerequisites (Data Lineage API androles/datalineage.producer) moved to therunNotes in the README. The help text now ends with an absolute link to the lineage guide instead of the repo-relative path, sinceindex_help_testexpects the help to point to the guide.--jit-timeout: now says "Each JiT action gets its own fresh deadline" instead of referring tojitCode.
| export const jitTimeoutOption: INamedOption<yargs.Options, IRunArgs> = { | ||
| name: "jit-timeout", | ||
| option: { | ||
| describe: |
There was a problem hiding this comment.
In emitLineageOption, the parenthetical (requires datalineage.googleapis.com enabled and roles/datalineage.producer, see docs/lineage.md) prints a repo-relative path in dataform help run and duplicates the Notes bullet in cli/README.md (consider moving the prerequisites to Notes like in compile and format). In jitTimeoutOption, consider replacing the internal proto field name jitCode with user-facing wording like Each JiT action.
There was a problem hiding this comment.
Same as above, addressed together with the --emit-lineage comment.
What
The command list, usage lines and argument/option tables in
cli/README.mdare now generated from the same command/option definitions that the CLI registers with yargs. They live between invisible markers:Everything outside the markers (command descriptions, Notes, exit codes, credentials file, examples) stays hand-written and is never touched. The markers don't render on GitHub or npm.
bazel run //cli:update_readmeregenerates the blocks.//cli:readme_generated_testfails if the README is out of date. It shows the differing lines with their README line numbers and explains how to fix them:Changes
Generator (
cli/readme_generator.ts, separate library, not bundled into@dataform/cli)commands,usage <cmd>,positionals <cmd>,options <cmd>,project-config-options.name,option.type,option.defaultandoption.describe, plus a new optionalINamedOption.docsfield (default,required) for values computed or validated at runtime (e.g.--timeout's default comes fromDEFAULT_COMPILATION_TIMEOUT_MILLIS).CLI_COMMANDSincli/commands/index.tsis the single list of commands used byrunCli()and the generator.Option definitions (user-visible in
dataform help)--actionsclaimed*wildcard support, butmatchPatterns()matches exactly;--varsreferenced the wrong key;format --actionsdescribed action names, but it takes file globs (now its own option object, same flag name, no behaviour change).checks are unchanged.type: "string"to--credentialsand the project-config options that had no type. Previously yargs parsed numeric-looking values as numbers (e.g.--credentials 1e3→1000,--schema-suffix 123→123); unset options are unaffected.README
CI
cli_docs) run as the last step of the GitHub Actionsbuild-linuxjob, which already builds//clifor every PR, including forks. Cloud Build only runs for external contributors after/gcbrunand its logs aren't public.scripts/run_tests_on_cloudbuild) to avoid running them twice; a local./scripts/run_testsstill runs them.contributing.mddocuments the workflow.Testing
bazel test //cli/... --test_tag_filters=-integration: all pass, including the newreadme_generator_test(generator unit tests on fixtures: each block kind, project-config collapsing, escaping, idempotency, error messages) andreadme_generated_test.bazel run //cli:update_readmeis idempotent.update_readmerestores the file byte for byte../scripts/run help <command>shows the updated descriptions.For reviewers
dataform helptext changes are incli/common_options.ts,cli/commands/*_options.ts,cli/commands/format_command.tsandcli/project_config_options.ts.build-linuxjob as a required check onmain, so an out-of-date README blocks merging.