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

[FIX] Fix EnumValidator #91

Merged
merged 3 commits into from
Apr 8, 2021
Merged

Conversation

Irallia
Copy link
Collaborator

@Irallia Irallia commented Mar 19, 2021

Resolves #78

Update EnumValidator such that args.methods (detection_methods) can be used and give a nice help page view.

EDIT (22.03.2021): We should probably wait for seqan/seqan3#2464 and update the seqan3 library.
EDIT (06.04.2021): We also want to wait for seqan/seqan3#2502 to avoid another seqan3 version change.
EDIT (07.04.2021): Both seqan PRs are merged now. This PR can get merged after the reviews.

@Irallia Irallia self-assigned this Mar 19, 2021
@Irallia Irallia requested a review from eseiler March 19, 2021 16:06
Comment on lines 294 to 289
{
"[Error] Value parse failed for -m: Argument 9 could not be parsed as type std::string.\n"
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This unhelpful error message is fixed by issue seqan/seqan3#2464.

…s) can be used and give a nice help page view.

Signed-off-by: Lydia Buntrock <[email protected]>
Copy link
Collaborator

@eldariont eldariont left a comment

Choose a reason for hiding this comment

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

Looks very good. I only have one tiny comment about the test cases.

{
cli_test_result result = execute_app("iGenVar");
std::string expected
std::string expected_res
Copy link
Collaborator

Choose a reason for hiding this comment

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

This local variable has the same name as the global variable above. Maybe rename to expected_res_no_options?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or we move the global variable into the test cases which use it? I guess we could decide to either define all result and error messages above the test cases or inside each test case. A mix of both is confusing.

Copy link
Collaborator

@eldariont eldariont left a comment

Choose a reason for hiding this comment

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

Thanks!

Irallia added 2 commits April 8, 2021 11:54
Signed-off-by: Lydia Buntrock <[email protected]>
@Irallia Irallia force-pushed the FIX/fix_EnumValidator branch from 1b56376 to 1c9d468 Compare April 8, 2021 09:55
@Irallia Irallia merged commit 8f75be8 into seqan:master Apr 8, 2021
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.

[BUG] Possible problem with the enum validator
3 participants