Skip to content

fix(utils): quiet read.output defaults - #4163

Merged
infotroph merged 6 commits into
PecanProject:developfrom
tyagivibhor111-hub:fix-3996-read-output-pr
Nov 5, 2026
Merged

infotroph merged 6 commits into
PecanProject:developfrom
tyagivibhor111-hub:fix-3996-read-output-pr

Conversation

@tyagivibhor111-hub

@tyagivibhor111-hub tyagivibhor111-hub commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Description

This PR makes read.output() quieter by default to reduce unnecessary informational messages during normal execution.

Previously, read.output() printed a summary by default and emitted informational messages about reading output for all years, listing NetCDF files, and reading all variables. These messages can make the output noisy when users do not need detailed logging.

This change disables summary printing by default and displays these routine informational messages only when verbose = TRUE. Important warnings and diagnostics, including invalid or missing origin-year warnings and missing output-file messages, are preserved.

Issue #3996

Changes Made

  • Changed the default value of print_summary from TRUE to FALSE.
  • Made the informational messages about reading all years, listing NetCDF files, and reading all variables conditional on verbose = TRUE.
  • Preserved important warnings and diagnostics.
  • Updated the read.output() documentation to reflect the new default behavior.
  • Updated tests to verify default logging behavior, verbose logging, and summary output when print_summary = TRUE.

Testing

  • Ran test-read.output.R using testthat.
  • Test result: 41 passed, 0 failed, 0 warnings, 0 skipped.
  • Ran git diff --check; no whitespace errors were reported.

Type of Change

  • Behavior improvement
  • Documentation update
  • Tests added or updated

Checklist

  • Updated documentation to reflect the new default.
  • Added or updated tests for the changed behavior.
  • Relevant tests pass.
  • No whitespace errors detected.
  • Reviewed the final changes on GitHub.
  • Ready for maintainer review.

@tyagivibhor111-hub

Copy link
Copy Markdown
Contributor Author

Hi @divine7022,@dlebauer, Could you please take a look when you get a chance and let me know if any changes are needed?

@infotroph

Copy link
Copy Markdown
Member

@tyagivibhor111-hub How did you decide who to tag here?

@tyagivibhor111-hub

Copy link
Copy Markdown
Contributor Author

@tyagivibhor111-hub How did you decide who to tag here?

Hi @infotroph, sorry for the confusion. I mistakenly thought the issue was raised by @divine7022 because I had worked on one of their issues before. I got confused while tagging, my apologies for that.
Could you please take a look at my PR above and let me know if anything needs to be changed? Thanks for your time!

@tyagivibhor111-hub

tyagivibhor111-hub commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

@infotroph, could you please review (#4163) when you get a chance? The checks are passing, and I’ve tested the changes with 41 passed and 0 failed. I’ve also preserved the important warnings and diagnostics while making routine informational messages conditional on verbose = TRUE. If any changes are needed. I’ll address your feedback promptly.

@tyagivibhor111-hub

Copy link
Copy Markdown
Contributor Author

@infotroph, could you please review #4163 when you get a chance and let me know if any changes are needed?

Comment on lines +131 to +133
expect_true(any(grepl("No start or end year provided", verbose_log)))
expect_true(any(grepl("Reading the following files", verbose_log)))
expect_true(any(grepl("Variables not specified", verbose_log)))

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.

use expect_match here as the above block was doing before

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok @infotroph , I’ll replace these expect_true(any(grepl(...))) assertions with expect_match, as suggested.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@infotroph ,Updated the assertions to use expect_match() as suggested. The test file passes locally.

Commit: cfc23a9

Comment thread base/utils/R/read.output.R Resolved
Comment thread base/utils/R/read.output.R Resolved
@infotroph
infotroph enabled auto-merge November 5, 2026 21:56
@infotroph
infotroph added this pull request to the merge queue Nov 5, 2026
Merged via the queue into PecanProject:develop with commit 279bdfd Nov 5, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants