Skip to content

Refactor/unify terms for clarity#16

Merged
DhruvSrikanth merged 7 commits intodevfrom
refactor/unify-terms-for-clarity
May 24, 2025
Merged

Refactor/unify terms for clarity#16
DhruvSrikanth merged 7 commits intodevfrom
refactor/unify-terms-for-clarity

Conversation

@yuxiang-wu
Copy link
Copy Markdown
Contributor

No description provided.

This commit refactors the CLI to align with the new API and database terminology, replacing "session" with "run".

Key changes:
- In `weco/api.py`:
    - Renamed API client functions (e.g., start_optimization_session -> start_optimization_run).
    - Updated API endpoint URLs and parameter names (session_id -> run_id).
- In `weco/cli.py`:
    - Renamed variables (e.g., session_id -> run_id, current_session_id_for_heartbeat -> current_run_id_for_heartbeat).
    - Updated log directory structure to use `run_id`.
    - Updated calls to renamed functions in `weco/api.py`.
    - Changed user-facing text and internal logic.
    - Corrected initialization of the tree panel for the first node.
    - Added defensive handling for the 'nodes' list from API responses.
- In `weco/panels.py`:
    - Updated `SummaryPanel` to use `run_id` instead of `session_id`.

These changes ensure the CLI consistently uses the new "run" terminology.
@DhruvSrikanth DhruvSrikanth requested review from DhruvSrikanth and Copilot and removed request for Copilot May 20, 2025 16:12
@DhruvSrikanth DhruvSrikanth added the documentation Improvements or additions to documentation label May 20, 2025
Previously, the `report_termination` call in the signal handler
used the default API timeout (30 seconds). This could lead to
significant delays when terminating the CLI (Ctrl+C) if the
network was slow or the server unresponsive.

This commit reduces the timeout for this specific call to 3 seconds,
prioritizing a faster exit for the user over guaranteed delivery
of the termination report in adverse network conditions.
Copy link
Copy Markdown
Member

@DhruvSrikanth DhruvSrikanth left a comment

Choose a reason for hiding this comment

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

The changes to variable names make sense.

Requested Changes:

  • Keep all comments that have been removed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think most (if not all) comments in this file are helpful since theres quite a bit going on in this file. I think its better to keep all of the comments.

Copy link
Copy Markdown
Member

@DhruvSrikanth DhruvSrikanth left a comment

Choose a reason for hiding this comment

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

Added comments back

@DhruvSrikanth DhruvSrikanth merged commit d6cef55 into dev May 24, 2025
1 check passed
@yuxiang-wu yuxiang-wu deleted the refactor/unify-terms-for-clarity branch May 27, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants