Skip to content

Feature/jpype script migration - #23

Merged
adamkorynta merged 3 commits into
mainfrom
feature/jpype-script-migration
Aug 21, 2026
Merged

Feature/jpype script migration#23
adamkorynta merged 3 commits into
mainfrom
feature/jpype-script-migration

Conversation

@adamkorynta

Copy link
Copy Markdown
Member
  • adds in basic smoke test against scripts to ensure API's are reachable from the wheel without moving to integration test territory

@adamkorynta
adamkorynta requested review from SWF-WM and krowvin July 24, 2026 22:14
@adamkorynta
adamkorynta force-pushed the feature/jpype-script-migration branch from 194ce49 to 958df67 Compare July 24, 2026 22:16
@adamkorynta
adamkorynta marked this pull request as ready for review July 24, 2026 22:21
@rma-psmorris
rma-psmorris self-requested a review July 28, 2026 20:57

@SWF-WM SWF-WM left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good

krowvin
krowvin previously approved these changes Aug 7, 2026

@krowvin krowvin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good x2

Base automatically changed from feature/jpype-github-actions to main August 17, 2026 23:09
@adamkorynta
adamkorynta dismissed krowvin’s stale review August 17, 2026 23:09

The merge-base changed after approval.

@adamkorynta
adamkorynta force-pushed the feature/jpype-script-migration branch from 958df67 to a30a554 Compare August 17, 2026 23:09

@rma-psmorris rma-psmorris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

compute_All_Flowgroups and compute_Single_Flowgroup are duplicated across several of the GateFlow scripts. Does it make sense to move these into a shared module rather than defining them as nested functions in each script? If this makes sense and is outside of the scope of this effort, capture it as a ticket? -- Although, capturing as a ticket would have the cat out of the box by the time we revise possibly making it too late to take advantage of.

Note that milestone 4 validates the scripts converted to the JPype workflow.

Comment thread regi-headless/src/test/python/test_district_scripts.py
Comment thread district-scripts/SWF/GateSettings.py
Comment thread district-scripts/SWL/WhiteR-InflowCalcMultipleActions.py
Comment thread regi-headless/src/test/python/test_district_scripts.py
Comment thread district-scripts/SWL/Big3-InflowCalcMultipleActions.py
Comment thread district-scripts/SWL/Big3-InflowCalcMultipleActions.py
@adamkorynta

adamkorynta commented Aug 19, 2026

Copy link
Copy Markdown
Member Author

compute_All_Flowgroups and compute_Single_Flowgroup are duplicated across several of the GateFlow scripts. Does it make sense to move these into a shared module rather than defining them as nested functions in each script? If this makes sense and is outside of the scope of this effort, capture it as a ticket?

The redundant code is purely the logic for error handling, which is the responsibility of individual scripts and not the library. We could write a library method that iterates over an array and then return a list of exceptions, but I don't see a large benefit or reduction in code duplication there (but allows callers to completely ignore errors....).

One other consideration that I don't like all that much is adding into regi_python_logging is a helper like:

@contextmanager
def log_errors(logger, description):
    try:
        yield
    except Exception:
        logger.exception("Error %s", description)

@rma-psmorris rma-psmorris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also reviewed and approved stacked PRs

@adamkorynta
adamkorynta merged commit 783e9d7 into main Aug 21, 2026
2 checks passed
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.

4 participants