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

Introduce ConstraintExpression #174

Merged
merged 6 commits into from
Jan 2, 2020

Conversation

Dreamescaper
Copy link
Member

@Dreamescaper Dreamescaper commented Dec 17, 2019

Created classes ConstraintExpression and ConstraintPartExpression, and moved methods from AssertExpressionHelper there.

ConstraintExpression represents the whole constraint expression (e.g. Is.Not.Null & Is.EqualTo("1").Or.EqualTo("A").IgnoreCase), and has property ConstraintPartExpression[] ConstraintParts.

ConstraintPartExpression represents single part of constraint expression, which are combined using binary operators (| or &) or constraint expression operator properties (.And or .Or).
(e.g. Is.Not.Null, Is.EqualTo("1"), EqualTo("A").IgnoreCase)
ConstraintPartExpression has three main properties:

  • PrefixExpressions - expressions which go before actual constraint method/property expression (Not, None, Count, Property(...) etc).
  • RootExpression - actual constraint method/property expression (Null, EqualTo("1") etc.)
  • SuffixExpressions - expressions which go after actual constraint expression (IgnoreCase, Within(...), Using(...) etc)

Created draft PR to show idea and for discussion. If suggested approach makes sense, I will add tests, add docs, etc.

@mikkelbu
Copy link
Member

Thanks for supplying this @Dreamescaper. I'm been busy due to the holidays, but hopefully I can give this a proper look tomorrow.

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

At first when I read the above description I thought that ConstraintPartExpression should be named ConstraintExpressionPart, but looking through the code I think that ConstraintPartExpression is the right choice.

I like that we are removing some of the technical details from the analyzers and into classes that represents concepts, and I think that this is a step in the right direction, so I very much think that the approach makes sense (and hopefully we can simplify more in later PRs).

I've left some minor comments - mostly nitpick.

@Dreamescaper Dreamescaper changed the title [WIP] Introduce ConstraintExpression Introduce ConstraintExpression Dec 30, 2019
@Dreamescaper Dreamescaper marked this pull request as ready for review December 30, 2019 17:56
@Dreamescaper
Copy link
Member Author

Updated PR with tests, added docs, minor changes.

Happy New Year :)

@mikkelbu
Copy link
Member

Also Happy new year from my end. I'll take a look at this PR next year :)

Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

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

I just corrected one typo. Otherwise, I think it looks great. Thanks

@mikkelbu mikkelbu merged commit 0fbaf4f into nunit:master Jan 2, 2020
@mikkelbu mikkelbu added this to the Release 0.2 milestone Apr 13, 2020
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.

2 participants