SEZ9 commented on PR #11586: URL: https://github.com/apache/seatunnel/pull/11586#issuecomment-5358315984
Thanks @DanielLeens for keeping the status of this one clear — I fully agree with the summary above, and nothing has changed since the previous review pass. To the contributor: the current head (`cb3017b6`) is still blocked on the same items raised earlier, so to avoid this getting lost, here are the concrete remaining asks before we can do a meaningful re-review: 1. **Move the code into the real module** — the top-level `path/` directory is not a valid source root. The `OptionRule`/`Conditions` changes need to live under the existing `seatunnel-api` module with proper package declarations and imports (`Map`, `List`, `HashMap` are currently missing). 2. **Fix the Java syntax** — the files still use Python-style `#` comments, which won't compile. 3. **Fix the validation logic bugs** — in particular, `Builder.required(...)` reassigns `rule = new OptionRule()` on each call, which drops previously added conditions. 4. **Enable CI in your fork** per @davidzollo's guidance above — note this is necessary but not sufficient, since CI will fail regardless until the head compiles. No need to reply point by point — once you push an updated commit with the code reworked under `seatunnel-api`, we'll re-review the full diff against the new head. Feel free to ask here if anything is unclear, and thanks again for contributing to Apache SeaTunnel! <!-- streview-comment:395 --> -- 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]
