Skip to content

Conversation

@ferenc-csaky
Copy link
Collaborator

Relevant changes

  • Added a shorthand for every CLI argument and updated README accordingly.
  • Refactored the environment variable handling utility, and moved all relevant logic into EnvVarUtils class.
  • Replaced assertions to AssertJ in every test file.

@ferenc-csaky ferenc-csaky requested a review from mbroecheler June 11, 2025 18:08
@ferenc-csaky ferenc-csaky force-pushed the refactor-and-housekeeping branch from c28fbaa to 4ed7b5d Compare June 11, 2025 18:11
@mbroecheler mbroecheler requested review from velo and removed request for mbroecheler June 11, 2025 19:34
@velo velo requested a review from Copilot June 12, 2025 12:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR performs a general refactor and cleanup across the codebase, adding shorthand CLI flags, centralizing environment‐variable logic, and standardizing test assertions.

  • Add short (-p, -s, -c, -u, -m) options for all command‐line arguments and update README.
  • Move all environment‐variable parsing into a single EnvVarUtils class and introduce JSON support.
  • Replace JUnit5 Assertions calls with AssertJ in every test.

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

File Description
README.md Updated CLI argument table, added shorthand flags, bumped version
flink-sql-runner/src/main/java/com/datasqrl/EnvVarUtils.java Extracted and improved env var resolution logic, added JSON support
flink-sql-runner/src/main/java/com/datasqrl/FlinkMain.java / SqlExecutor.java / JsonEnvVarDeserializer.java Switched to EnvVarUtils methods and removed old helpers
All test files (various under src/test/java) Migrated from JUnit assertions to AssertJ for consistency
Comments suppressed due to low confidence (2)

flink-sql-runner/src/test/java/com/datasqrl/SqlRunnerTest.java:36

  • In setUp(), udfPath is never initialized before being used in the CLI args array, resulting in a null argument. Either initialize udfPath to a valid path or remove the --udfpath flag from the test arguments.
private String udfPath;

flink-sql-runner/src/main/java/com/datasqrl/EnvVarUtils.java:55

  • [nitpick] Consider using StringBuilder instead of the synchronized StringBuffer here, since this method is single-threaded and will have better performance.
var res = new StringBuffer();

Copy link
Collaborator

@velo velo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferenc-csaky
Copy link
Collaborator Author

LGTM.

Not sure if you used this: https://docs.openrewrite.org/recipes/java/testing/assertj/assertj-best-practices

Nope, coding agent and myself, but most asserts are pretty straightforward so not much room for AssertJ goodness (yet).

@ferenc-csaky ferenc-csaky force-pushed the refactor-and-housekeeping branch from 69308cc to 3553323 Compare June 12, 2025 12:48
@ferenc-csaky ferenc-csaky merged commit fa94c90 into main Jun 12, 2025
2 checks passed
@ferenc-csaky ferenc-csaky deleted the refactor-and-housekeeping branch June 12, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants