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.

Sign up for GitHub

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

Jump to bottom

Move validating conventions to the model validator #15659

Merged
merged 1 commit into from
May 13, 2019
Merged

Move validating conventions to the model validator #15659

merged 1 commit into from
May 13, 2019

Conversation

ajcvickers
Copy link
Contributor

Issue #4016

Also make sure validation logging is sent to the validation category, which means that DiagnosticsLoggers can be removed.

There are some convention that do work and can throw exceptions. These have not been moved.

@ajcvickers ajcvickers requested a review from AndriySvyryd May 8, 2019 19:05
@ajcvickers ajcvickers force-pushed the ThrowingOnTheTowel0505 branch from 0c659d4 to 426fb00 Compare May 8, 2019 19:47
AndriySvyryd
AndriySvyryd requested changes May 8, 2019
View reviewed changes
Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

:octocat:

@ajcvickers
Copy link
Contributor Author

@AndriySvyryd General question. Currently model validation runs on IModel (as opposed to IMutableModel or IConventionModel.) Given that those methods in your comments above should not be on the read-only types, what is the appropriate thing to do in the model validator?

@AndriySvyryd
Copy link
Member

@ajcvickers It runs on IModel to indicate that it won't alter the model in any way, but its purpose is to run only on mutable models, since we assume that readonly models were already validated. So I think it's ok to downcast if needed.

@AndriySvyryd
Copy link
Member

Strictly speaking we should introduce another readonly interface that provides access to all information from a mutable model, but I think that's overkill.

@ajcvickers
Copy link
Contributor Author

@AndriySvyryd Sounds good

@ajcvickers ajcvickers force-pushed the ThrowingOnTheTowel0505 branch from 426fb00 to ada2dc6 Compare May 11, 2019 21:31
@ajcvickers ajcvickers requested a review from AndriySvyryd May 11, 2019 23:59
AndriySvyryd
AndriySvyryd requested changes May 12, 2019
View reviewed changes
Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

:octocat:

@ajcvickers ajcvickers force-pushed the ThrowingOnTheTowel0505 branch from ada2dc6 to 47a75b7 Compare May 13, 2019 00:05
@ajcvickers ajcvickers requested a review from AndriySvyryd May 13, 2019 00:06
@ajcvickers
Move validating conventions to the model validator
09cc32a 
Issue #4016

Also make sure validation logging is sent to the validation category, which means that `DiagnosticsLoggers` can be removed.

There are some convention that do work and can throw exceptions. These have not been moved.
@ajcvickers ajcvickers force-pushed the ThrowingOnTheTowel0505 branch from 47a75b7 to 09cc32a Compare May 13, 2019 15:04
@ajcvickers ajcvickers merged commit 41622d1 into master May 13, 2019
@ghost ghost deleted the ThrowingOnTheTowel0505 branch May 13, 2019 17:40
@ajcvickers ajcvickers mentioned this pull request May 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndriySvyryd AndriySvyryd AndriySvyryd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ajcvickers @AndriySvyryd