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.

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

A field with a default value set to nil should not be enforced #14

Closed
lasseebert opened this issue Mar 17, 2020 · 7 comments
Closed

A field with a default value set to nil should not be enforced #14

lasseebert opened this issue Mar 17, 2020 · 7 comments
Labels
T:Bug Type: Bug

Comments

@lasseebert
Copy link

When the struct has enforce: true, one can override this by setting a default value on a field. Something like this:

typedstruct enforce: true do
  field :non_enforced_field, integer, default: 42
end

But it is not possible to default to nil (it will not un-enforce the field:

typedstruct enforce: true do
  # This will not work. The field is still enforced
  field :non_enforced_field, integer | nil, default: nil
end

I know that adding enforce: false on the field will do the same thing, since nil is the default value for non-enforces fields, but for the explicitness and readability, it would be nice the set the default value to nil.

If you agree, @ejpcmac, I will be happy to give it a shot.

@ejpcmac
Copy link
Owner

ejpcmac commented Mar 17, 2020

Hello @lasseebert,

An enforced value must be non-nil. By setting enforce: true globally, you mean that :non_enforced_field is in fact enforced. The way you define it says the same thing as doing:

defmodule Test do
  @enforce_keys [:non_enforced_field]
  defstruct non_enforced_field: nil

  @type t() :: %__MODULE__{non_enforced_field: integer() | nil}
end

Which is obviously invalid: you cannot create a %Test{} with :non_enforced_field set to nil.

In your first example, the enforce: true is not overriden: the field is still enforced, but since it has a default value, your user do not need to provide a non-nil value.

@lasseebert
Copy link
Author

@ejpcmac

Thanks for the explanation :)
I thought setting a default value would unset the global enforce: true for that field (ref the docs):

A key with a default value is not enforced.

which is why I expected

typedstruct enforce: true do
  field :non_enforced_field, integer | nil, default: nil
end

to generate something like

defmodule Test do
  @enforce_keys []
  defstruct non_enforced_field: nil

  @type t() :: %__MODULE__{non_enforced_field: integer() | nil}
end

@ejpcmac
Copy link
Owner

ejpcmac commented Mar 17, 2020

I thought setting a default value would unset the global enforce: true for that field (ref the docs):

A key with a default value is not enforced.

Mmm. Maybe that’s a documentation issue then. I’ll fix it in the next release.

@lasseebert
Copy link
Author

Thanks for your quick responses. It all makes sense :)
I will consider this issue closed, but I will let you close it in case you want to leave it open for the doc changes.

@ejpcmac ejpcmac added the T:Documentation Type: Documentation label Mar 17, 2020
@ejpcmac ejpcmac changed the title Be able to set default to nil Document properly the behaviour of default when a field is enforced Mar 17, 2020
@ejpcmac
Copy link
Owner

ejpcmac commented Mar 17, 2020

@lasseebert You’re welcome. Please let it open so I can remember it.

@ejpcmac ejpcmac added T:Bug Type: Bug and removed T:Documentation Type: Documentation labels May 27, 2020
@ejpcmac ejpcmac changed the title Document properly the behaviour of default when a field is enforced A field with a default value set to nil should not be enforced May 27, 2020
@ejpcmac
Copy link
Owner

ejpcmac commented May 27, 2020

Mmm. In fact that’s a bug. Reading my tests, I have :

test "does not enforce keys for fields with a default value" do
  refute :with_default in EnforcedTypedStruct.enforce_keys()
end

test "does not enforce keys for fields with a default value set to `false`" do
  refute :with_false_default in EnforcedTypedStruct.enforce_keys()
end

So I’ll add a test for the default value set to nil. That’s a corner case. Property testing would have catch it, but I haven’t got the time to figure how to property-test code generation.

@lasseebert
Copy link
Author

Awesome 👍 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T:Bug Type: Bug
Projects
Status: Solved
Development

No branches or pull requests

2 participants