-
Notifications
You must be signed in to change notification settings - Fork 150
Warn on invalid prop/data defaults #374
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
Conversation
test/properties_test.exs
Outdated
| """ | ||
|
|
||
| assert output =~ ~S""" | ||
| prop `invalid_acc2` default value 3 must be a list when accumulate: true |
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.
should i wrap the value in "``" like i do for the prop/data name?
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, I think we should start using ` ` whenever we have code-related stuff, just like we do in markdown. If in the future, we want to turn those boring monochromatic messages into beautiful colourful ones (like in Rust), it won't have to update any of the messages.
Maybe this is also valid for https://github.com/surface-ui/surface/pull/374/files#diff-fb7dcaf7105c49bd4b887f35e2c32958116d06d3ef6639370ed6cb3741f287cbR491 for accumulate: true.
msaraiva
left a comment
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.
@davydog187 looks great to me!
test/properties_test.exs
Outdated
| """ | ||
|
|
||
| assert output =~ ~S""" | ||
| prop `invalid_acc2` default value 3 must be a list when accumulate: true |
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, I think we should start using ` ` whenever we have code-related stuff, just like we do in markdown. If in the future, we want to turn those boring monochromatic messages into beautiful colourful ones (like in Rust), it won't have to update any of the messages.
Maybe this is also valid for https://github.com/surface-ui/surface/pull/374/files#diff-fb7dcaf7105c49bd4b887f35e2c32958116d06d3ef6639370ed6cb3741f287cbR491 for accumulate: true.
|
@msaraiva updated the formatting. Are you suggesting to make a change for |
msaraiva
left a comment
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.
@davydog187 looks great!
Do you think we should enhance the message a bit so we can instruct the user how to fix the issue? Something like:
prop `type` default value `"x-large"` does not exist in `:values!`
Hint: Either choose an existing value or replace `:values!` with `:values` to skip validation.
|
yea, i like that idea |
|
@msaraiva I added the hint, should be good to go |
|
@davydog187 thanks! ❤️ |
* surface-next: Remove `phx_` prefix from props (surface-ui#384) Remove code for embedded interpolation inside strings Introduce <#unless> (surface-ui#376) Introduce the concept of root properties (surface-ui#382) Warn on invalid prop/data defaults (surface-ui#374) Deprecated unquoted strings (surface-ui#379) Convert embedded interpolation (surface-ui#380) Introduce <#raw> and deprecate <#Raw> (surface-ui#377)
Builds off of #314
Warns when a prop's default value is not in the
:values!list