-
Notifications
You must be signed in to change notification settings - Fork 20
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
Warning for unknown args with tests #967
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.
Can't see anything wrong with it.
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.
This needs a way to suppress the warning messages via a config property really to close the issue, as otherwise it just adds unsuppressable warning spam to any models wich just want to add to the default CLI (e.g. the use case which led to me opening the issue).
This doesn't need to be done prior to a release however, but either needs the PR changing to not automatically close the issue, or a new issue opening about suppressing the warning.
Otherwise it does allow the intended use case to work (passing a custom --population N
arg to the extreme agent scale test model I wrote, while still allowing -s
etc to be used.)
Added a CLI option to suppress unknown args to avoid opening an new issue. |
Corresponding docs PR |
Fixes #931.
Testing for previous behaviour is tricky as it causes a system exit. There are exit tests available in google test but these don't support "test for no exit". It would be possible to have a friend class test the arg parsing function specifically however an exit from google test would not constitute a pass (it just wouldn't fail as the testing would exit with an error code) so this seems unnecessary.