-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add weir #485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I'm no longer intimately familiar with the code base, this looks good to me -- the change is also somewhat minor as the weir is arguably just a Pump alias.
I think "weir" is maybe not the best term: it's explicitly a weir that maintains the level. I don't think the name has to be a blocker, but maybe it's worth keeping an issue for it.
What would be good to change before merging is to resolve the auto-generated code coverage remarks. Ideally the output is tested as well, but this is often difficult (hence 100% coverage doesn't mean 100% reliable). However, given that Julia is a dynamic language, it would be good that (nearly) all lines run during a test.
Many of the changes in this PR also affect |
Fixes #347.
See #482 what has to be done related to this.