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

Add metadata strings for operators and improve CLI output #102

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

yane3628
Copy link
Contributor

@yane3628 yane3628 commented May 4, 2021

@yane3628 yane3628 added this to In progress in May - 2021 via automation May 4, 2021
@yane3628
Copy link
Contributor Author

yane3628 commented May 4, 2021

Structured expression messages still need to be implemented.

@yane3628 yane3628 marked this pull request as draft May 4, 2021 00:17
@yane3628 yane3628 marked this pull request as ready for review May 4, 2021 02:38
@yane3628 yane3628 marked this pull request as draft May 4, 2021 18:30
@yane3628 yane3628 self-assigned this May 5, 2021
May - 2021 automation moved this from In progress to Review in progress May 13, 2021
Copy link
Contributor

@JohnathonMohr JohnathonMohr left a comment

Choose a reason for hiding this comment

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

I think changing the design to have messages finalized inside the Expression and Operator (when the Result is populated) will help the code flow more smoothly. I'm also thinking about what the messaging for the not operator would look like. I'm not sure how best to represent that yet, but something we need to decide before committing to a design here.

internal static string InFailureMessage = $"Value \"{ExpectedValuePlaceholder}\" is not in the list at path \"{PathPlaceholder}\".";

internal static string AnyOfFailureMessage = $"No evaluations evaluted to true for the following json property: \"{PathPlaceholder}\".";
internal static string AllOfFailureMessage = $"One or more evaluations were false for the following json property: \"{PathPlaceholder}\".";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if each of these was implemented as a method directly in the corresponding Operator, and the placeholders would be filled in using parameters to the method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, maybe we could update how EvaluateExpression() works, and either create the string and have it ready in another property after evaluation, or return the string as part of the function call.

/// <summary>
/// Gets the messsage which explains why the evaluation failed.
/// </summary>
public string FailureMessage()
Copy link
Contributor

Choose a reason for hiding this comment

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

As much as possible, I think all this logic should be in the Operators and Expressions. I think it would be easier to understand and write if the failure messages were built right where the evaluations happen.

@yane3628 yane3628 removed this from Review in progress in May - 2021 May 21, 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.

Add metadata to result to help identify exactly what part of the check was violated
2 participants