Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bmi_Adapter: drop 'StreamHandler output' #840

Merged

Conversation

PhilMiller
Copy link
Contributor

@PhilMiller PhilMiller commented Jun 26, 2024

The entire hierarchy of classes below Bmi_Adapter takes StreamHandler output as a constructor argument, and just passes it up the chain. The only places it was actually used was in a pair of re-initialization warnings.

Changes

  • Change the warnings to use logging::warning instead
  • Drop the Bmi_Adapter::output member that was otherwise unused
  • Drop the StreamHandler output parameter from all class constructors in the hierarchy
  • Remove output arguments used to fill those parameters in adapter construction calls

Testing

  1. ctest
  2. CI

Notes

  • The Formulation classes are minimally changed, and particularly did not get into modifying their interfaces

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

program--
program-- previously approved these changes Jun 26, 2024
Copy link
Contributor

@program-- program-- left a comment

Choose a reason for hiding this comment

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

Looks good to me, the comments are nitpicks if anything. I'll wait to merge till tomorrow so there's a bit more visibility, since this might affect some in-dev PRs (i.e. Forcings Engine ones). Should be quick/easy changes for those though.

include/bmi/Bmi_Adapter.hpp Outdated Show resolved Hide resolved
include/bmi/Bmi_Py_Adapter.hpp Outdated Show resolved Hide resolved
@PhilMiller
Copy link
Contributor Author

I just test-merged with your Forcings Engine PR branch, and found a couple missing headers. Added the <iostream> here, and that'll need a utilities/StreamHandler.hpp.

PhilMiller added a commit to program--/ngen that referenced this pull request Jun 26, 2024
@PhilMiller PhilMiller merged commit db9bc20 into NOAA-OWP:master Jun 26, 2024
19 checks passed
@PhilMiller PhilMiller deleted the PhilMiller/bmi-adapter-output-stream branch June 26, 2024 16:29
program-- pushed a commit to program--/ngen that referenced this pull request Jun 26, 2024
program-- added a commit to program--/ngen that referenced this pull request Jun 26, 2024
PhilMiller added a commit to program--/ngen that referenced this pull request Jun 27, 2024
PhilMiller added a commit to program--/ngen that referenced this pull request Jun 27, 2024
program-- added a commit that referenced this pull request Jul 1, 2024
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.

2 participants