SEZ9 commented on PR #11586: URL: https://github.com/apache/seatunnel/pull/11586#issuecomment-5140456948
Thanks @davidzollo for the CI guidance and @DanielLeens for keeping the review state clear — agreed on both points. To the contributor: please note that enabling CI in your fork is necessary but won't be sufficient here. The current head `cb3017b61fe1` cannot compile as-is, so CI would fail regardless. Concretely, before we can do a meaningful re-review, the following needs to be addressed: 1. **Move the code into a real module** — the top-level `path/` directory is not a valid source root. The `OptionRule`/`Conditions` changes belong under the existing `seatunnel-api` module with proper package declarations and imports (`Map`, `List`, `HashMap` are all missing). 2. **Fix the Java syntax** — the files currently use Python-style `#` comments, which won't compile. 3. **Fix the validation logic bugs** — `Builder.required(...)` reassigns `rule = new OptionRule()` on each call and drops prior conditions; `GreaterThanCondition` compares a value against itself due to parameter shadowing; `Required/OptionalCondition` never look up the field name, so required options are effectively never validated; `ConditionalCondition` compares the config value to the field-name string. 4. **Clarify the `Condition.check` contract** — it's currently inconsistent between scalar and `Map` inputs with unchecked casts. 5. **Move the test** — the JUnit 4 test under `src/main` should be a JUnit 5 test under `src/test`, matching the project's conventions. 6. **Align with the migration goal** — #11007 asks for migrating connectors/transforms to declarative validation; introducing a parallel string-keyed `OptionRule` that shadows the existing typed one needs discussion first. Ideally this builds on (or extends) the existing typed API rather than duplicating it, and demonstrates the approach on at least one real connector. Once a new commit is pushed addressing these, we'll re-review the full diff against the new head. Happy to help if you hit any issues along the way. <!-- streview-comment:12 --> -- 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]
