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

Warning for unknown args with tests #967

Merged
merged 2 commits into from
Nov 9, 2022
Merged

Warning for unknown args with tests #967

merged 2 commits into from
Nov 9, 2022

Conversation

mondus
Copy link
Member

@mondus mondus commented Nov 7, 2022

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.

Copy link
Member

@Robadob Robadob left a 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.

Copy link
Member

@ptheywood ptheywood left a 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.)

@mondus
Copy link
Member Author

mondus commented Nov 8, 2022

Added a CLI option to suppress unknown args to avoid opening an new issue.

@mondus
Copy link
Member Author

mondus commented Nov 8, 2022

Corresponding docs PR

@mondus mondus self-assigned this Nov 9, 2022
@mondus mondus merged commit 9f8c6bf into master Nov 9, 2022
@mondus mondus deleted the unknown_args branch November 9, 2022 15:07
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.

CLI: Optionally allow unknown arguments
3 participants