I think the time/effort required to fix the bad code to use MyStruct.new etc is exaggerated. It’s got to be less than 30mins. Surely we should spend the time to fix our bugs.
cmo
4
mudasobwa
Creator of Cure
Yeah, Lazy should have been deprecated and removed, I simply did not have time to bother.
That’s because the library is not about calculated values (btw, they are truly calculated, not dynamic defaults; when any of values it depends on has changed, the value in calculated field(s) is also re-calc’ed.)
This library is about handy generation od property-data for property-based testing in the first place, it just supports calculated values by accident.
I am not sure how would you expect dependencies to automagically convert to support dynamic defaults, let alone it might break them. For a big production project, I would typically comment out the declaration of the struct, collect all compilation errors, and fix them one by one. Changing them to a constructor would probably suffice your needs, but I might be mistaken.
0
ffloyd
Isn’t 29 mins for adding a dynamic default… too much? It’s less than minute in many other languages and a pretty common feature.
And making defaults dynamic in Ecto is definitely more than 30 mins.
0
ffloyd
For example Estructura. With my current proposal I would just use it with defdynstruct. It still generates __schema__ functions, etc. I expect all the features of Estructura to just work. Am I right or it somehow relies on %A{} == %A{}? (that’s the only promise that is broken by defdynstruct).
Is it adequate price for introducing dynamic default?
0
a-maze-d
Even though I do understand the ask, I’m not sure it’s a good idea. Especially the %A{} == %A{} argument is a very strong one. Thus we shouldn’t touch the raw struct.
Also, I don’t really see the big value of struct/2 over a A.new/1
Not to mention that a defdynstruct sounds very clunky.
You could have an annotation like @init :myinit (if you like with some syntactic sugar with name convention so you only need to define only an __init__ function), BUT I would even vote against that, because of the classical rule:
don’t surprise
The developer would need to know that you do dynamic initialisation. IMHO it’s better to be explicit.
As a library creator you should think carefully on whether you want to have statically or dynamically created structs. Provide a new/1 if you might want to extend it to a dynamic struct in the future (apply SOLID principles). In that case you might not want to expose the actual data structure.
2
BartOtten
Don’t know but don’t think so. This thread already took more time; so does reviewing the changes, testing etc.
Using a factory seems to be a pretty nice option; making the data structure an implementation detail.
0
FlyingNoodle
I appreciate the initiative, I really do. For a language to survive and thrive it needs people to think about it, tinker with it, criticize it, etc. So thanks for that.
However, I’m not a fan of this proposal for two reasons:
- I like the fact that elixir does not have as many concepts to learn as other languages. We should be very careful with adding more complexity.
- The fact that
%A{} == %A{}wouldn’t work anymore is killer.
2
ffloyd
I see. %A{} != %A{} is kind of ugly thing, I agree. I like the principle “a cure should not be worse than a disease”. I proposed a cure, but it has this side effect. Therefore, if we agree that disease is worse than having %A{} != %A{}, the proposal has a chance. So, let me show symptoms of “disease” more explicit.
My initial problem was about “Ecto cannot do dynamic defaults”. There is no proper workaround. autogenerate has pretty different behavior and does not work for virtual fields. I was thinking about proposing something like this:
defmodule Reminder do
use Ecto.Schema
schema "users" do
field :remind_at, :datetime, default_fn: &today_plus_10_days/0
# ... rest of the fields ...
end
end
Then I thought about implementation. Usually we create a record like this:
%Reminder{}
|> create_changeset()
Ecto also explicitly allows to do:
Repo.insert(%Reminder{})
Where this “default_fn” should be invoked? There is no guarantee that changeset will be used so we cannot put it into changeset. Patch all changeset functions and Repo functions to invoke “default_fns”? Even if we do this we have another problem: people do factories in the following way:
def reminder_factory(...) do
%Reminder{
user: build(:user)
}
end
How to make “default_fn” to work here?
If even creators of Ecto (and ExMachina) fell into the trap that locked them from providing reliable dynamic defaults without huge refactoring - it means we have a problem in the language design.
This is the disease I’m fighting with here. All the code in this message is idiomatic and comes from widely used libraries. It’s not about mere “struct creation with defaults”, it’s about preventing libraries from supporting dynamic defaults.
While I do not like introducing %A{} != %A{} I still see it as an acceptable tradeoff. Also, “blast radius” is controllable: you accept this tradeoff explicitly when prefer defdynstruct over defstruct.
The fact that default: DateTime.utc_today() is resolved at compile time actually also a surprise of a similar level. In my experience people constantly do this mistake. By using defdynstruct you replace one surprise with another, you do not increase overall amount of surprises (but I agree we increase variety).
Did I change how you see the problem? =)
0
LostKobrakai
Tbh I disagree with the idea of Repo.insert(%Reminder{}) needing to support dynamic defaults as well. Elixir is very much about being explicit. If you need your data to come from side effects I expect the code to tell me. %Reminder{} |> with_default_target_datetime() |> Repo.insert() is a lot better code than Repo.insert(%Reminder{}) automatically messing around with struct fields.
You’re argueing that struct defaults being a compile time thing prevent libraries from introducing dynamic field defaults. I’d counter that with dynamic defaults being something you want to have an explicit api for and once you do there’s no longer any limitation. I don’t expect ecto ever wanted to provide you with a dynamic defaults feature to begin with.
In the end your argument comes down to “I want to introduce dynamically calculated default values without changing my codebase” – not a great argument imo, such a change should be made explicit by changing code.
7
ffloyd
I think this is the root point of misalignment between us. I believe that in practice this particular explicitness only makes development more expensive, and makes code more complex and harder to support without real benefits. But there cannot be a theoretical proof for this.
Let’s wait for more feedback then!
Meanwhile I’m curious, what would be you idea of implementing dynamic default with Ecto? Patching all the functions that uses schema or can have this schema module as an argument? Or do I miss something more clever?
0