chovy-3012 commented on PR #12514:
URL: https://github.com/apache/seatunnel/pull/12514#issuecomment-5882604366
> # What Problem Does This PR Solve?
> * **User pain point:** Anyone reading `CosFile.md` (source connector) sees
an `Options` table with no `Description` column, so two options
(`filename_extension`, `sort_files_by_modification_time`) have no inline
explanation in the table itself — readers have to scroll down to the prose
sections to learn what they do.
> * **Fix approach:** Add a `Description` column (header + separator; the
Chinese file's header cell holds the corresponding Chinese word for
"Description") to the `Options` table in both
`docs/en/connectors/source/CosFile.md` and
`docs/zh/connectors/source/CosFile.md`, and fill in English description text
for the two previously-undocumented rows.
> * **One-sentence summary:** This PR adds a `Description` column to the
CosFile source options table and documents two previously-bare rows in English,
but leaves the Chinese table's new column empty everywhere.
>
> # 1. Code Change Review
> ## 1.1 Core Logic Analysis
> This is a documentation-only change — 2 files, 6 insertions/6 deletions,
no code, config, or registration touched.
>
> **Before (en, from `dev`):**
>
> ```
> | name | type | required | default value
|
>
|----------------------------|---------|----------|-----------------------------|
> ...
> | filename_extension | string | no | -
|
> ...
> | sort_files_by_modification_time | boolean | no | false
|
> ```
>
> **After (en, this PR):**
>
> ```
> | name | type | required | default value
| Description |
>
|----------------------------|---------|----------|-----------------------------|-------------|
> ...
> | filename_extension | string | no | -
| Filter files by the specified file extension, e.g. `csv`, `.txt`,
`json`, or `.xml`. |
> ...
> | sort_files_by_modification_time | boolean | no | false
| Whether to sort files by modification time in descending order.
When enabled, schema inference uses the latest file when reading evolving
schemas. |
> ```
>
> **Key findings (verified against `origin/dev` and against PR #12499, which
was merged into `dev` just before this PR was opened, on the same day, and
specifically audited all file-connector docs "against source code"):**
>
> 1. The PR description states the two rows "already contained a 5th column
with description text" that was "silently dropped during rendering." I checked
`git show origin/dev:docs/en/connectors/source/CosFile.md` directly — this is
**not accurate**. Both rows had exactly 4 columns, same as every other row in
the table; there was no hidden/dropped 5th cell. So this PR isn't "restoring"
lost content — it's _newly authoring_ a description for those two options.
That's still a genuinely useful improvement, just not the root cause described
in the PR body. I'd only ask that the PR description be corrected so the commit
history doesn't carry a factually incorrect story forward (see Issue 1).
> 2. New content is correct and matches the existing prose sections further
down the same file (`### filename_extension [string]` and `###
sort_files_by_modification_time [boolean]`), so the added English text is
accurate, not invented.
> 3. Markdown table syntax is valid: the new header row and separator row
both have 5 pipe-delimited columns in both en and zh files, so this won't
produce a malformed/broken table. GFM table parsing pads any row with fewer
cells than the header as empty cells — it does not break rendering, it just
leaves those cells blank.
> 4. **Parity gap:** the zh diff only touches the header + separator line (2
additions/2 deletions total) — no Chinese description text was added for the
`filename_extension` or `sort_files_by_modification_time` rows. After this PR,
`docs/zh/connectors/source/CosFile.md`'s new Description column will be empty
for literally every row in the table, including the two rows this PR is meant
to document. This directly contradicts the PR's own stated goal: "The en
version also gets matching English descriptions for the same two rows to keep
en/zh consistent." See Issue 2.
> 5. Scope check: no other tables/examples in either file are touched or
affected; the change is fully localized to the `## Options` table's
header/separator plus two rows.
>
> No engine/API/runtime path is involved, so no call-chain or state-machine
diagram is needed here.
>
> ## 1.2 Compatibility Impact
> **Fully compatible.** Pure documentation text change; no config option,
API, protocol, or default value is touched.
>
> ## 1.3 Performance / Side-Effect Analysis
> None. No runtime code is affected.
>
> ## 1.4 Error Handling and Logging
> Not applicable (no code path), but see the two content-accuracy issues
below, which are the substantive issues in this PR.
>
> * **Issue 1**
>
> * **Location:** PR description (not a file diff)
> * **Problem description:** The stated root cause ("body rows already
contained a 5th column with description text... silently dropped during
rendering") does not match the actual pre-PR content of
`docs/en/connectors/source/CosFile.md` / `docs/zh/connectors/source/CosFile.md`
— verified directly against `origin/dev`. Both target rows had exactly 4
columns, matching the header, with no dropped content.
> * **Potential risk:** Low — this doesn't affect the shipped doc, but it
leaves an inaccurate narrative in the PR/commit history that could confuse
future readers trying to understand "what bug was fixed here."
> * **Best improvement:** Update the PR description to say the new column
and descriptions are newly added (not recovered/restored).
> * **Severity:** Low
> * **Raised by another reviewer:** No
> * **Issue 2**
>
> * **Location:** `docs/zh/connectors/source/CosFile.md:56,81,93`
> * **Problem description:** The zh table gains a new Description header
column, but no Chinese description text is added to any row — including the two
rows this PR is meant to document (`filename_extension` line 81,
`sort_files_by_modification_time` line 93). This leaves the zh table's new
column empty everywhere, which contradicts the PR's own stated
en/zh-consistency goal.
> * **Potential risk:** Low functional risk (an empty cell isn't wrong
information, just missing information, same as before), but it does mean
Chinese-reading users get strictly less documentation than English-reading
users after this change — a real, if small, doc-parity regression relative to
the PR's stated intent.
> * **Best improvement:** Add a one-line Chinese description to both rows.
For `sort_files_by_modification_time`, a ready Chinese sentence already exists
a few dozen lines below, in the `### sort_files_by_modification_time [boolean]`
prose section of the same zh file, and can be reused/shortened directly.
`filename_extension` doesn't have an existing zh prose section to draw from (a
separate, pre-existing gap, not introduced by this PR), so a short translation
of the new English cell text would need to be authored.
> * **Severity:** Medium
> * **Raised by another reviewer:** No
>
> # 2. Code Quality Assessment
> ## 2.1 Coding Standards
> Not applicable — no code, only Markdown table cells.
>
> ## 2.2 Test Coverage and Test Stability
> Not applicable — pure documentation change, no UT/E2E touched, none
expected.
>
> ## 2.3 Documentation Updates
> Covered above. One more non-blocking observation:
>
> * **Issue 3**
>
> * **Location:** `docs/en/connectors/source/CosFile.md` (whole `##
Options` table, ~33 of 35 rows)
> * **Problem description:** After this PR, only 2 of the ~35 option rows
have a `Description` value; the rest render with an empty cell. This is a
pre-existing gap (not introduced by this PR) but it's inconsistent with sibling
connector docs such as `OssFile.md`, `S3File.md`, and `SftpFile.md`, where
every option row in the equivalent table has a description.
> * **Potential risk:** None immediate; purely a
documentation-completeness/consistency observation.
> * **Best improvement:** Consider a follow-up PR to fill in descriptions
for the remaining options in `CosFile.md` (en and zh), matching the fuller
sibling docs. Not required for this PR.
> * **Severity:** Low
> * **Raised by another reviewer:** No
>
> # 3. Architectural Soundness
> ## 3.1 Elegance of the Solution
> Precise, minimal, well-targeted diff for the stated scope (2 rows, both
files) — good scoping discipline. The only gap is that the "both files" part
isn't actually symmetric in content yet (Issue 2).
>
> ## 3.2 Maintainability
> Trivial Markdown edit, no maintainability concerns.
>
> ## 3.3 Extensibility
> Not applicable to a docs-only change.
>
> ## 3.4 Historical-Version Compatibility
> Fully compatible — no behavior, config, or protocol is affected, so there
is nothing to migrate and no impact on any released version.
>
> # 4. Issue Summary
> Number Issue Location Severity
> Issue 2 zh table gets a new Description header but no Chinese
description text anywhere, contradicting the PR's own en/zh-consistency goal
`docs/zh/connectors/source/CosFile.md:56,81,93` Medium
> Issue 1 PR description's claimed root cause (dropped 5th column)
doesn't match the actual pre-PR file content PR description Low
> Issue 3 Most other rows in the table still have no description
(pre-existing, not introduced here) `docs/en/connectors/source/CosFile.md`
Low
> # 5. Merge Recommendation
> ### Conclusion: Ready to merge after fixes
> 1. **Blockers — must be fixed**
>
> * Issue 2 (Medium): please add a short Chinese description to the
`filename_extension` and `sort_files_by_modification_time` rows in
`docs/zh/connectors/source/CosFile.md` so the new Description column isn't
empty on every row — this is the one place where the PR doesn't yet deliver on
its own "keep en/zh consistent" goal. Should be a two-sentence addition.
> 2. **Recommended fixes — non-blocking**
>
> * Issue 1 (Low): tweak the PR description so it doesn't claim
pre-existing content was "silently dropped" — the two rows simply didn't have a
description before, same as the rest of the table.
> * Issue 3 (Low): consider a follow-up to fill in the remaining option
descriptions for full parity with sibling connector docs (`OssFile.md`,
`S3File.md`, `SftpFile.md`).
>
> Otherwise this is a clean, low-risk, well-scoped docs improvement — thanks
for taking the time to make the CosFile options table clearer! CI is green and
the table markup itself is syntactically valid in both files, so once the zh
parity gap above is closed this should be good to go.
Thanks for flagging this.
The Chinese description text for both rows already exists in the file and
was present before this PR:
```md
Line 80 (filename_extension): 使用指定的文件扩展名筛选文件，例如 csv、.txt、json 或 .xml。
Line 92 (sort_files_by_modification_time): 是否按修改时间降序排序文件。启用此选项后，在读取不断演化的
schema 时可确保 schema 推断使用最新的文件。
```
These descriptions were already there , they just weren't rendering.
Without a Description column in the header.
Adding the header column is what makes the existing content render correctly.
--
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]
