Warn if given default value doesn't exist in values list

[yusufyildirim]

Hi,

This PR's scope discussed a bit in #312. I've found a couple of cases:

• We should be checking if the given default value exists in the values list (for data and prop and if not accumulated)
  Example (should give warning): prop type, :string, values: ["small", "medium", "large"], default: "x-large"

• We should be checking if the given default value(s) exist in the value list (only for prop and if accumulated)
  Example(should give warning): prop count, :integer, default: [3], values: [0, 1, 2], accumulate: true

• Should be valid
  prop count, :integer, default: [], values: [0, 1, 2], accumulate: true

There are some more cases but that is the core idea.

I'm an Elixir newbie and this is why I struggled to bring idiomatic Elixir code to the table 🙈I definitely need your feedback and this code definitely needs splitting. Please, guide me through if you have time.

Thank you in advance!

[msaraiva]

Hi @yusufyildirim!

Thanks for the PR. Before moving on with this change, we need to decide how strict the values option will be. Currently, those values are mostly for docs and tooling, e.g. the catalogue, code complete, etc. There's no validation which means the user can set any value of the same type. This might be the desired behaviour. For instance, if you define a prop, let's say color, you might want to provide a set of predefined colours like red, blue, black, whatever, but leave the possibility for the user to set any other valid colour, like: #00DD00 or #FFFF00. So we can't simply always emit a warning. We need alternatives. Here are some for consideration:

1. Create an additional, mutual exclusive option in regard to values, let's say suggestions. The difference is that unlike values , suggestions are not validated.
2. Extend the syntax of values to accept something like: values: ["red", "green", "blue', ...]. The 3 dots, indicates that there are other valid values so we don't warn.

Any other alternative we should consider?

BTW, this differentiation is even more important if we want to start validating properties passed as literals in the markup. Normally, you'd define a default value that is already on the list of values, however, non-default values passed by the user may not be predictable.

[miguel-s]

I always assumed values were strict and would be validated. All my use-cases have fallen into this category so far, but you make a very good point with the colours example.

I like option 1, the naming is clear enough to me. I'm surprised option 2 is even possible and I believe it would cause confusion, as it's not very standard Elixir.

Another option I can think of is to have a boolean strict option that toggles validation in values (and maybe others, too?).

[msaraiva]

I'm surprised option 2 is even possible and I believe it would cause confusion, as it's not very standard Elixir.

@miguel-s that notation is valid Elixir code. That syntax is mostly used to define type specs. For example, the following type is valid:

@type my_list :: [integer, ...]

Where [integer, ...] is just a shorthand for nonempty_list(integer)

However, I agree it might be confusing, although it's very handy :)

[yusufyildirim]

I didn't think that way 😄 I assumed values should be validated too. Elixir is a dynamic language and the current behavior is pretty reasonable. But I think having some sort of strict option would be helpful too. Introducing a new option like strict: true may not be that handy though. I'm okay with ... notation or something similar that doesn't introduce a new option to add everywhere I put :values 😄

[msaraiva]

Another option would be to keep values and add values!. The latter, obviously, validate it :)

[msaraiva]

Alright. Time to vote!

Strict and non-strict versions:

1. values and suggestions
2. values and values!
3. values: ["v1", "v2"] and values: ["v1", "v2", ...]

/cc @surface-ui/core

[yusufyildirim]

I'd say 2. is the most obvious and least verbose 😄

[Malian]

Alright. Time to vote!

Strict and non-strict versions:

1. `values` and `suggestions`

2. `values` and `values!`

3. `values: ["v1", "v2"]` and `values: ["v1", "v2", ...]`

/cc @surface-ui/core

3

[lnr0626]

Alright. Time to vote!

Strict and non-strict versions:

1. values and suggestions
2. values and values!
3. values: ["v1", "v2"] and values: ["v1", "v2", ...]

/cc @surface-ui/core

2, but only a slight preference over 3

[msaraiva]

Although I slightly prefer #3 over #2, I believe the former will be more intuitive to the user. Besides, it has the benefit of not introducing a breaking change as it keeps the current behaviour of values. So I guess #2 is the winner!

@yusufyildirim feel free to move on and let's know if you need any assistance.

Thanks for working on this!

[msaraiva]

Hi @yusufyildirim!

Just to make it clear, we're only checking if the default value is in the list if the user sets values!, not values.

We'll also need to find a way to indicate this difference values vs values! in the docs. For instance, the Card component from SurfaceBootstrap has a width prop. Although it suggests one of 25, 50, 75, 100, it can actually be any percentage value so it should keep it as values, not values!.

The first thing that comes to my mind is to add , ... to the end of the list, like:

25, 50, 75, 100, ...

Any other idea?

Another 2 things we could validate in a separate PR:

1. If all values in the list are valid according to the specified type. This validation can be applied to both, values and values!.
2. If a prop value passed as literal is in the list. e.g. <Card text_align="not_in_list">. Note: It should only be validated when using values!.

[yusufyildirim]

Hi @msaraiva

Yes, everything is clear. I didn't have time to work on this for now BTW.

The first thing that comes to my mind is to add, ... to the end of the list, like:

Totally makes sense. I'm a bit worried about series of numbers though. Adding ... to the end of a sequence of numbers makes me feel like there is a pattern going on. For example, seeing ... at the end of a sequence like 5, 10, 15, 20, ... might make me think if like the next possible number would be 25, because there is a pattern 😄 This is a bit related to how ... works in math I believe 😄

I don't know if I explained it well but this might worth thinking a bit. Except that, it's ok.

[msaraiva]

Adding ... to the end of a sequence of numbers makes me feel like there is a pattern going on.

Yeah. It could give that impression. Another option could be:

25, 50, 75, 100 or a valid :integer

or maybe add a prefix "Suggestions":

Suggestions: 25, 50, 75, 100

But that's quite verbose. Not sure.

[yusufyildirim]

I don't know, maybe we should just add a sentence or a paragraph that explains what ... means, and assuming people read the explanation, we use ... everywhere in the docs. People not going to read it anyway... 😄

[davydog187]

Hello @yusufyildirim, do you need help with this PR? I've similarly become interested in this problem, as well as adding some possibilities with ranges in values so you can do things like

prop age, :integer, values!: 1..100

[davydog187]

Regardless of my suggestion, which probably is a separate topic, I'm happy to take over this PR to push it over the line.

[yusufyildirim]

Hello @davydog187, I'm pretty busy these days -even weekends- and this is why I couldn't find time to implement this. If you want to continue, please feel free to take over

[davydog187]

Great, and I'll add support for my current PR #355

Thank you @yusufyildirim

[davydog187]

Started work on this :)

[davydog187]

I just opened #374

[yusufyildirim]

Closing this one, thank you @davydog187!