Skip to content

Try statements - fixes for ruff rule PLW0717#865

Merged
rnyb merged 11 commits into
mainfrom
try-statements
May 26, 2026
Merged

Try statements - fixes for ruff rule PLW0717#865
rnyb merged 11 commits into
mainfrom
try-statements

Conversation

@rnyb
Copy link
Copy Markdown
Collaborator

@rnyb rnyb commented May 24, 2026

Closes #864

Copy link
Copy Markdown

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 refactors several code paths to comply with Ruff rule PLW0717 (“too many statements in try clause”), primarily by shrinking try-block bodies and extracting helper functions, addressing #864.

Changes:

  • Refactored summaryplot interactive menu handling and vector/datafile splitting to reduce try-clause size.
  • Extracted restart unpack/slice/repack logic into a helper function in restartthinner.
  • Refactored fmuobs.autoparse_file() into small _try_parse_* helpers and simplified parser selection flow.
  • Adjusted a permission-handling test to move setup/assertions out of a large try block.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/test_fmu_copy_revision.py Moves setup and assertions out of a large try block; adds a small assertion helper.
src/subscript/summaryplot/summaryplot.py Shrinks try scope; extracts interactive menu loop into a helper function.
src/subscript/restartthinner/restartthinner.py Extracts unpack/slice/repack operations into a helper to reduce try size.
src/subscript/fmuobs/fmuobs.py Splits format detection into _try_parse_* helpers to reduce try sizes and improve flow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/subscript/summaryplot/summaryplot.py Outdated
Comment thread src/subscript/summaryplot/summaryplot.py Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 24, 2026

Codecov Report

❌ Patch coverage is 80.59701% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.55%. Comparing base (43c40f7) to head (7f21b43).

Files with missing lines Patch % Lines
src/subscript/summaryplot/summaryplot.py 71.05% 11 Missing ⚠️
src/subscript/fmuobs/fmuobs.py 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #865      +/-   ##
==========================================
+ Coverage   84.51%   84.55%   +0.03%     
==========================================
  Files          49       49              
  Lines        7349     7374      +25     
==========================================
+ Hits         6211     6235      +24     
- Misses       1138     1139       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Break loop on empty stdin read (EOF condition)
- Join terminated plot process with timeout before restarting
- Force kill process if it doesn't terminate within 5 seconds
Copy link
Copy Markdown

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread src/subscript/summaryplot/summaryplot.py Outdated
Comment thread src/subscript/summaryplot/summaryplot.py Outdated
Copy link
Copy Markdown

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/subscript/fmuobs/fmuobs.py:217

  • pd.to_datetime(dframe["DATE"]) is now outside the try/except ValueError. If a file looks like the internal CSV format but contains an invalid DATE value, this will raise and stop autoparse_file instead of falling through to other parsers. Wrap the DATE conversion in the same try/except (or use errors="coerce" and validate) to avoid breaking format auto-detection.
        )
        if "DATE" in dframe:
            dframe["DATE"] = pd.to_datetime(dframe["DATE"])
        return ("csv", dframe)

src/subscript/fmuobs/fmuobs.py:236

  • obsdict2df(obsdict) is now outside the try/except guarding YAML parsing. If YAML loads successfully but conversion to dataframe fails with ValueError, autoparse_file will now error out rather than trying other formats. Consider including the conversion step in the protected block (or catching conversion errors) to keep auto-detection resilient.
    if isinstance(obsdict, dict) and (
        obsdict.get("smry", None) or obsdict.get("rft", None)
    ):
        logger.info("Parsed %s as a YAML file with observations", filename)
        return ("yaml", obsdict2df(obsdict))

Comment thread tests/test_fmu_copy_revision.py Outdated
Comment thread src/subscript/fmuobs/fmuobs.py Outdated
- Wrap terminal mode change and interactive menu in try/finally
- Use filedesc consistently instead of calling fileno() twice
- Add proper subprocess cleanup with timeout and forced kill
@rnyb rnyb force-pushed the try-statements branch from 1dc593e to e787af3 Compare May 24, 2026 22:53
…error handling

- Move format-checking logic inside try blocks to catch all related exceptions
- Broaden exception handling to include OSError alongside ValueError
- Add debug logging for failed parse attempts
- Initialize dframe variable before try block in _try_parse_ert
- Fix grammar in test docstring
Copy link
Copy Markdown

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread src/subscript/summaryplot/summaryplot.py Outdated
Comment thread src/subscript/summaryplot/summaryplot.py
Copy link
Copy Markdown

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

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

Comment thread src/subscript/summaryplot/summaryplot.py Outdated
Comment on lines +588 to +589
datafiles.append(vecdata)
summaryfiles.append(sumfn)
Comment thread src/subscript/fmuobs/fmuobs.py Outdated
Comment thread src/subscript/fmuobs/fmuobs.py Outdated
Copy link
Copy Markdown

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

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

Copy link
Copy Markdown

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread src/subscript/fmuobs/fmuobs.py Outdated
Copy link
Copy Markdown

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread src/subscript/fmuobs/fmuobs.py Outdated
Copy link
Copy Markdown

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

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

@rnyb rnyb force-pushed the try-statements branch from 139dbec to 487c9cd Compare May 25, 2026 13:48
Copy link
Copy Markdown

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

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

@rnyb rnyb requested review from alifbe and larsevj May 25, 2026 14:13
Copy link
Copy Markdown
Collaborator

@alifbe alifbe left a comment

Choose a reason for hiding this comment

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

Nice 👍🙏

Looks good to me.

@rnyb rnyb merged commit 58c26f2 into main May 26, 2026
13 checks passed
@rnyb rnyb deleted the try-statements branch May 26, 2026 07:21
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.

Refactor code to support ruff rule PLW0717

4 participants