Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Validation improvements #149
Validation improvements #149
Changes from 7 commits
5b99ad5
f159ca0
a2ab6b7
f2b2957
28c4941
2105da1
268e9d8
b000121
2bd44d5
f36abf1
f1afc91
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you extract out a
validate_property()
fromvalidate_properties()
and then use that here? Then if we add more validation checks we don't have to keep these in syncThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that too. It's just pretty marginal since it's only used in two places and the logic is v simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first two thoughts were:
new_class(validator=)
but it might be nice to mention that here somehow)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've expanded the
validator
docs innew_class()
and linked to them from here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were designing this function, this is where I'd put
...
to force named arguments, becausevalidate(x, FALSE)
is essentially meaningless to me without the argument name.Plus this feels like a function which has a decent chance that we will add more arguments to it, and that would free us from caring about argument order.
Not sure how we feel about this principle for R7 though, since it will be tied closely to base R
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed with both points. But since this is going in base R (which doesn't use that pattern), and
properties
is mostly their for internal use, I'll leave as is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding an unneeded
...
to the argument list also comes with a price. I would never want to pay that just to force users..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit formal arguments also improve code clarity. Users are free to follow whatever calling convention they like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the errors in the object validator are likely to be spurious, should we just go ahead and error immediately if any properties are invalid? Maybe it could have a header of
Invalid <obj> properties:
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see you've tried to do something like this by avoiding the object validator with the
length(errors) == 0
branch. Hmm, I think I still prefer an immediate error - but only because I would mention in the header that it was the properties that were invalid (I don't feel strongly about this if you want to keep it as is)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good idea.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.