-
Notifications
You must be signed in to change notification settings - Fork 211
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
Allow missing property for opExample #4499
base: main
Are you sure you want to change the base?
Allow missing property for opExample #4499
Conversation
All changed packages have been documented.
Show changes
|
@@ -1567,7 +1567,13 @@ function checkExampleValid( | |||
diagnosticTarget, | |||
); | |||
if (!assignable) { | |||
program.reportDiagnostics(diagnostics); | |||
// We exclude missing-property diagnostic because some properties might not be present in certain protocol due to visibility. |
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.
This seems like a pretty crude approach. This will allow any opExample
invocations that have missing properties to work, but we don't really want that behavior since it will allow missing properties that actually should be there because they're required and present in the visibility transform.
I think we really ought to run the visibility transform and then try to assign the value to the transformed type.
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 if you feel positive that with the new visibility proposal this will just work then I think we should do that as well.
I did it this way because I planned to do that at the time of examples but forgot and there wasn't any solution for visibility at the time.
What do you think is the timeline for visibility, wondering if maybe we should just not do that as a warning to unblock those users for now at least.
fix #4240