zhangshenghang commented on PR #10395: URL: https://github.com/apache/seatunnel/pull/10395#issuecomment-3799444277
> ### Issue 1: External links in FAQ not updated > **Location**: > > * `docs/en/faq.md:113` > * `docs/zh/faq.md:119` > > **Related context**: > > * These FAQ files reference old contribution paths > * Old paths were deleted in PR [[DOC]Optimize Seatunnel Document Structure #10262](https://github.com/apache/seatunnel/pull/10262) > > **Issue description**: In the FAQ documentation, there are two references to old documentation paths: > > ``` > // docs/en/faq.md:113 > refer to the [SeaTunnel Contribution Guide](https://seatunnel.apache.org/docs/contribution/setup). > > // docs/zh/faq.md:119 > 具体参考：https://seatunnel.apache.org/docs/contribution/setup > ``` > > The documentation pointed to by these links has been moved to new locations in this PR: > > * New path: `/docs/developer/setup` > > If users click these old links, they will encounter 404 errors. > > **Potential risks**: > > * Users cannot navigate from FAQ to developer guide > * Reduces documentation usability and user experience > * If external websites or blogs reference these links, they will also become invalid > > **Impact scope**: > > * **Direct impact**: Navigation functionality of FAQ documentation > * **Indirect impact**: All users who rely on FAQ to find developer guide > * **Affected area**: Documentation website users (does not involve code functionality) > > **Severity**: MAJOR > > **Improvement suggestions**: > > ``` > // docs/en/faq.md:113 changed to: > refer to the [SeaTunnel Developer Guide](https://seatunnel.apache.org/docs/developer/setup). > > // docs/zh/faq.md:119 changed to: > 具体参考：https://seatunnel.apache.org/docs/developer/setup > ``` > > **Rationale**: > > * Maintain validity of documentation links > * Improve user experience > * Avoid external link failures > > ### Issue 2: Some valuable developer documentation still not restored > **Location**: > > * Documentation deleted in PR [[Doc] Seatunnel Doc Structure Adjustment #10351](https://github.com/apache/seatunnel/pull/10351): > > * `docs/en/developer/architecture/module-design.md` > * `docs/en/developer/architecture/project-structure.md` > * `docs/en/developer/connector-development/catalog-api.md` > * `docs/en/developer/connector-development/cdc-connector-guide.md` > * `docs/en/developer/connector-development/overview.md` > * `docs/en/developer/connector-development/sink-api.md` > * `docs/en/developer/connector-development/source-api.md` > * `docs/en/developer/e2e-testing.md` > * `docs/en/developer/engine-development.md` > * `docs/en/developer/transform-development.md` > > **Related context**: > > * These documents were deleted in PR [[Doc] Seatunnel Doc Structure Adjustment #10351](https://github.com/apache/seatunnel/pull/10351) (commit [e93bbb9](https://github.com/apache/seatunnel/commit/e93bbb9500a665e8ec80e6551eff73e74001e7ee)) > * This PR did not restore these documents > > **Issue description**: In the previous PR #10351, multiple subdirectories under the `developer/` directory were deleted, including: > > 1. `architecture/` - Architecture design documentation > 2. `connector-development/` - Connector development guide > 3. Other developer documentation (e2e-testing, engine-development, transform-development) > > These documents are very valuable for developers to understand SeaTunnel's architecture and development process. This PR only restored partial documents under the `developer/` directory (setup, coding-guide, etc.), but did not restore these subdirectories. > > **Potential risks**: > > * Developers cannot find architecture design and development guide documentation > * New contributors may have difficulty understanding SeaTunnel's module design > * Connector developers lack detailed API documentation > > **Impact scope**: > > * **Direct impact**: Developers who want to deeply understand SeaTunnel architecture > * **Indirect impact**: Increased onboarding difficulty for new contributors > * **Affected area**: Developer documentation (does not affect end users) > > **Severity**: MAJOR (considering these documents were just deleted in PR #10351, this may be intentional behavior) > > **Improvement suggestions**: If these documents are valuable, it is recommended to: > > 1. Restore these documents in a follow-up PR > 2. Or confirm that these documents are indeed no longer needed and explain the reason in the PR description > > **Rationale**: > > * Maintain documentation completeness > * Reduce onboarding difficulty for new contributors > * If these documents are indeed no longer needed, there should be a clear explanation > > **Note**: This issue needs confirmation from the community, as deleting these documents in PR #10351 may have specific reasons. It is recommended to ask maintainers in PR comments. > > ### Issue 3: Documentation changes not fully tested and verified > **Location**: > > * All newly added documentation files > > **Related context**: > > * The "How was this patch tested?" section in the PR description is empty > * No explanation of whether local documentation build testing was performed > > **Issue description**: This PR involves changes to 19 documentation files, including: > > 1. Adding multiple MD/MDX files > 2. Modifying `docs/sidebars.js` configuration > > However, the PR description does not specify: > > * Whether the Docusaurus website was built locally for verification > * Whether documentation rendering was checked to be normal > * Whether sidebar navigation was verified to work > * Whether links were checked to be valid > > **Potential risks**: > > * Documentation may have Markdown/MDX syntax errors > * Sidebar configuration may have issues > * May fail during documentation website build > * May have dead links > > **Impact scope**: > > * **Direct impact**: Documentation website build and deployment > * **Indirect impact**: Users cannot access documentation normally > * **Affected area**: Documentation website > > **Severity**: MAJOR > > **Improvement suggestions**: Add test description in the PR description, for example: > > ``` > ### How was this patch tested? > > 1. Built the documentation site locally using `npm run start` in the `docs/` directory > 2. Verified all new pages render correctly > 3. Checked the sidebar navigation works as expected > 4. Verified no broken links in the new documents > ``` > > Or execute the following commands before submitting: > > ```shell > cd docs > npm install > npm run build > # Check if the build succeeded > ``` > > **Rationale**: > > * Ensure documentation changes do not break website build > * Increase confidence in code review > * Comply with Apache project quality standards > > ### Issue 4: Maven command inconsistency in setup.md > **Location**: > > * `docs/en/developer/setup.md:35` > * `docs/zh/developer/setup.md:34` > > **Related context**: > > * In the `Install Subproject Locally` section > * Historical version used: `./mvnw clean install -DskipTests` > * This PR modified to: `./mvnw install -Dmaven.test.skip` > > **Issue description**: In the `developer/setup.md` documentation, this PR changed the Maven command from: > > ```shell > ./mvnw clean install -DskipTests > ``` > > to: > > ```shell > ./mvnw install -Dmaven.test.skip > ``` > > Although these two commands are functionally equivalent (both skip tests), there are the following issues: > > 1. Removing the `clean` parameter may cause incremental compilation problems > 2. Parameter format inconsistency (`-DskipTests` vs `-Dmaven.test.skip`) > > **Potential risks**: > > * Not cleaning may cause local builds to use old build artifacts > * Parameter inconsistency may confuse developers > > **Impact scope**: > > * **Direct impact**: Developers following documentation to set up development environment > * **Indirect impact**: May cause local build failures or abnormal behavior > * **Affected area**: Development environment setup > > **Severity**: MINOR > > **Improvement suggestions**: Recommend using one of the following commands (maintain consistency): > > ```shell > # Option 1: Recommended (consistent with historical versions) > ./mvnw clean install -DskipTests > > # Option 2: Also acceptable (but keep clean) > ./mvnw clean install -Dmaven.test.skip > ``` > > **Rationale**: > > * `clean` parameter ensures clean build, avoiding interference from old artifacts > * Maintain consistency with historical versions > * `-DskipTests` is more concise and widely used elsewhere in the project @misi1987107 Please fix issues 1 and 4 -- 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]
