bitflicker64 commented on code in PR #472:
URL: https://github.com/apache/hugegraph-doc/pull/472#discussion_r3954363692
##########
scripts/hugo.sh:
##########
@@ -0,0 +1,162 @@
+#!/bin/sh
+set -eu
+
+usage() {
+ printf '%s\n' \
+ "Usage: scripts/hugo.sh server [Hugo arguments...]" \
+ " scripts/hugo.sh build [Hugo arguments...]"
+}
+
+if [ "$#" -eq 0 ]; then
+ usage >&2
+ exit 2
+fi
+
+mode=$1
+shift
+case "$mode" in
+ server|build) ;;
+ *)
+ usage >&2
+ exit 2
+ ;;
+esac
+
+script_dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)
+repo_dir=$(dirname "$script_dir")
+cd "$repo_dir"
+
+reject_argument() {
Review Comment:
⚠️ The wrapper's real job — generate the version config with
`scripts/versioning.py config` and pass it as `--config hugo.yaml,$tmp` — is
about thirty lines. The other sixty are a hand-rolled parser for Hugo's own
flag syntax: an `expect_value` state machine, four spellings each of
`--baseURL`/`-b` and `--port`/`-p`, and an eleven-entry reject list
(`--config`, `--gc`, `--minify`, `--panicOnWarning`, `--logLevel`,
`--environment`, …).
The reject list guards nothing. Lines 145-162 already put the wrapper's
flags before `"$@"`, and Hugo's pflag is last-wins, so a contributor passing
`--minify=false` already works — this turns that into `exit 2`. It is a local
preview script, not a trust boundary; nobody loses data because someone
previewed with `--logLevel debug`.
It is also most of what `scripts/test_hugo_wrapper.py` (219 lines) tests:
`test_owned_hugo_arguments_are_rejected_before_any_tool_runs`,
`test_site_origin_environment_cannot_conflict_with_base_url`, and
`test_base_url_and_port_keep_generated_and_hugo_origins_aligned` exist only for
this machinery.
Requested change: delete `reject_argument()` and the reject `case` arms
(lines 29-33 and 51-64) with those three tests, and collapse the sniff to the
two spellings README documents:
```sh
for argument in "$@"; do
case "$argument" in
--baseURL=*) base_url=${argument#*=} ;;
-p) port=next ;;
*) [ "$port" = next ] && port=$argument ;;
esac
done
```
`HG_DOC_SITE_ORIGIN` already covers every other origin case and is already
documented in `contribution.md`.
##########
tests/e2e/workflow-contract.test.cjs:
##########
@@ -0,0 +1,88 @@
+const assert = require("node:assert/strict");
+const fs = require("node:fs");
+const path = require("node:path");
+const test = require("node:test");
+
+const workflow = fs.readFileSync(
+ path.resolve(__dirname, "../../.github/workflows/hugo.yml"),
+ "utf8"
+);
+const versionManifest = JSON.parse(
+ fs.readFileSync(path.resolve(__dirname, "../../versions.json"), "utf8")
+);
+
+test("each build fetches and verifies its immutable matrix SHA", () => {
Review Comment:
⚠️ Six tests, and four of them assert that a YAML file says what the YAML
file says.
- This one matches `/git fetch --no-tags origin "\$RESOLVED_SHA"/` and `/git
cat-file -e .../` — the shell block quoted back at itself.
- `aggregate binds the option-looking artifact suffix` asserts
`--artifact-suffix="..."` and `doesNotMatch` the space-separated spelling. That
is an equals-sign check.
- `prepare pins Hugo and WebP tools before source validators` compares
`workflow.indexOf("name: Setup Hugo Extended")` against `indexOf("name:
Validate source and version tooling")`. Rename a step and it fails.
- `concurrency serializes every writer` is a `[\s\S]*` regex over the group
expression; reorder two keys and it fails.
The one invariant worth protecting is in `only publish receives write
permission` — but it is spelled against the inline-flow form `permissions: {
contents: read }`, so converting that to block style turns a no-op reformat
into a red build. CI runs this on every PR (`npm run test:ci`,
`.github/workflows/hugo.yml:353`), so those false failures are live, and the
next person to touch the workflow pays for them.
Requested change: keep the write-permission check and delete the other five
cases:
```js
test("only publish receives write permission", () => {
assert.equal((workflow.match(/contents: write/g) || []).length, 1);
assert.match(workflow, /publish:[\s\S]*?contents: write/);
});
```
About ten lines instead of eighty-eight, and it still catches the thing that
would actually matter. `git diff` already proves the rest.
##########
layouts/_partials/version-target.html:
##########
@@ -0,0 +1,63 @@
+{{- $p := .page -}}
+{{- $version := .version -}}
+{{- $versionID := $version.version | default ($version.name | urlize) -}}
+{{- $rawURL := $version.url | default "" -}}
+{{- $versionURL := "" -}}
+{{- if $rawURL }}{{ $versionURL = printf "%s/" (strings.TrimSuffix "/"
$rawURL) }}{{ end -}}
+{{- $target := $versionURL -}}
+{{- $equivalent := false -}}
+{{- $fallback := false -}}
+{{- $relative := strings.TrimPrefix "/" $p.RelPermalink -}}
+{{- $locale := $p.Site.Language.Lang -}}
+{{- $docsPrefix := cond (eq $locale "cn") "cn/docs/" "docs/" -}}
+{{- $docsSuffix := cond (eq $locale "cn") "/cn/docs" "/docs" -}}
+{{- $root := strings.TrimSuffix $docsSuffix (strings.TrimSuffix "/"
$versionURL) -}}
+{{- if and $rawURL (strings.HasPrefix $relative $docsPrefix) -}}
+ {{- $pages := hugo.Data.version_routes.pages | default dict -}}
Review Comment:
⚠️ The `range` on line 19 reverse-maps this page's path to a logical route
ID by walking every entry in `hugo.Data.version_routes.pages` — 195 entries at
head — and there is no `break`, so it runs to completion after it has already
found the match.
That would be fine once per page. It is not once per page:
`version-target.html` is called once per version from `navbar.html:87`,
`navbar.html:174`, `shell/sidebar-panel.html:69`, and `actions/manifest.html:3`
via `version-options.html`. Five versions × four call sites ≈ 3,900 map
comparisons per page, and the PR's own validation reports 1,082 HTML pages per
aggregate — so roughly four million, twenty times more than the one scan the
page needs.
Deriving the ID from the path instead is not an option and I am not asking
for it: 28 of the route cells deviate from `lang:path` (all `api-preformance` →
`api-performance`), which is the reason the map exists. But the answer is the
same for all twenty calls on a page.
Requested change, no new file and no route-map schema change:
```gotemplate
{{- $logicalID := $p.Store.Get "hgVersionLogicalID" -}}
{{- if not $logicalID -}}
{{- $logicalID = "" -}}
{{- range $candidateID, $candidateRoutes := $pages -}}
{{- if eq (index $candidateRoutes $currentVersion) $relative -}}{{-
$logicalID = $candidateID -}}{{- end -}}
{{- end -}}
{{- $p.Store.Set "hgVersionLogicalID" $logicalID -}}
{{- end -}}
```
`.Store` is already the pattern here — `actions/manifest.html:1` reads
`.Store.Get "tdOutputFormat"`. Adding `{{ break }}` inside the range is a
second one-word win. A `byPath` index from `generate_version_routes` would be
tidier, but `validate_version_routes` asserts an exact top-level key set, so
that one costs a validator and test change too.
##########
layouts/_partials/hooks/body-end.html:
##########
@@ -0,0 +1,54 @@
+{{- $basePath := (urls.Parse .Site.BaseURL).Path | default "/" -}}
+{{- $localePrefix := cond (eq .Site.Language.Lang "cn") "cn/" "" -}}
+{{- $docsRoot := printf "%s%sdocs/" $basePath $localePrefix -}}
+{{- $fallbackMessage := "" -}}
+{{- if eq .Site.Language.Lang "cn" -}}
+ {{- $fallbackMessage = "目标版本没有此页面，已转到该版本的文档首页。" -}}
+{{- else -}}
+ {{- $fallbackMessage = T "ui_version_fallback" -}}
+{{- end -}}
+{{- $shellConfig := dict
+ "version" (.Site.Params.version | default "latest")
+ "locale" .Site.Language.Lang
+ "docsRoot" $docsRoot
+ "versionFallbackMessage" $fallbackMessage
+-}}
+<script type="application/json" id="hg-shell-config">{{ $shellConfig | jsonify
| safeJS }}</script>
+{{- $shell := resources.Get "js/hugegraph-shell.js" -}}
+{{- if hugo.IsProduction }}{{ $shell = $shell | minify | fingerprint }}{{ end
}}
+<script src="{{ $shell.RelPermalink }}"
+ {{- with $shell.Data.Integrity }} integrity="{{ . }}"
crossorigin="anonymous"{{ end }}></script>
+
+{{- $ai := partial "ai/config.html" . -}}
+{{- if $ai.enabled -}}
+ {{- $lang := .Site.Language.Lang -}}
+ {{- $sourceGroup := index $ai.sourceGroups $lang -}}
+ {{- $themeColor := index .Site.Params.ui "theme_color" -}}
+ {{- $historical := ne (.Site.Params.version | default "latest") "latest" -}}
+ {{- $labels := cond (eq $lang "cn")
Review Comment:
⚠️ These four lines hardcode six Chinese UI strings in a Go template. Line 8
of this same file reads the English fallback message with `T
"ui_version_fallback"` — and then line 6, two lines earlier, hardcodes its
Chinese counterpart inline rather than adding the key to `i18n/cn.yaml`.
Both translation files are right there: this PR creates `i18n/en.yaml`, and
`i18n/cn.yaml` already carries 236 entries. Every other partial in this PR uses
`T` — `backlinks.html` calls it five times. The practical cost is that the
Chinese copy for a user-facing feature is not in the file a translator opens,
so `询问 AI` and `AI 暂时不可用，本地搜索不受影响。` will quietly drift.
Requested change: add `ui_ask_ai`, `ui_ask_ai_description`,
`ui_ask_ai_latest`, `ui_retry`, `ui_ai_error` and `ui_version_fallback` to both
YAML files, then:
```gotemplate
{{- $labels := dict "ask" (T "ui_ask_ai") "description" (T
"ui_ask_ai_description") "latest" (T "ui_ask_ai_latest") "retry" (T "ui_retry")
"error" (T "ui_ai_error") -}}
```
That deletes both `cond` branches here and lines 4-9 above, since
`$fallbackMessage` becomes `T "ui_version_fallback"` unconditionally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
