-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Turn unspecified defaults in at-kwdef into required kwargs #27987
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
9c97c0b to
1165849
Compare
simonbyrne
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.
Seems like a good idea.
I'm all in favor of the rest of the change, but I'm not sure about this one. Usually we generate constructors that accept any arguments and convert to the field types. |
|
Okay I've removed that change. |
| indexed_deltas::Cuint | ||
| received_bytes::Csize_t | ||
| total_objects::Cuint = Cuint(0) | ||
| indexed_objects::Cuint = Cuint(0) |
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 should work if you just do = 0, right?
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.
It didn't until I removed the assertion that Jeff mentioned, so it does work now. I'd rather not go back through and change all of these to be less explicit though.
|
Maybe add an explicit test for the auto-conversion? e.g. @test Test27970Typed(a=0x03) == Test27970Typed(3) |
|
Suggested test has been added. |
This comment has been minimized.
This comment has been minimized.
The existence of at-kwdef predates the existence of required keyword arguments; before that, all keyword arguments needed a default value. So currently when no value is specified for a field in the type definition given to kwdef, it's assigned a default value based on the field's type. With this change, any field not explicitly given a default becomes a required keyword argument in the resulting type constructor when using kwdef. In doing this, we're also able to drop the requirement that the type's fields be typed, and that the type has fields at all.
|
Anyone want to take another look? |
|
What do you guys think about, if the type is a mutable struct, then fields without default values which are not specified via keyword arguments remain undefined and can be set later? |
|
IMO that seems a little finnicky and easy to forget that the behavior differs based on whether the type is mutable. |
|
Also, "undefined" is a bit of weird edge case that we don't really want to encourage, or at the very least, we want to be explicit (hence the |
Yea, that's true. What about an option, say, julia> @kwdef allow_undef=true mutable struct Foo
x=2
y
end
julia> Foo()
Foo(2, #undef)This could work for both mutable/immutable (although in the latter case is likely less useful). Granted this probably significantly increases the complexity of writing the macro.
That's fair, although in this case AFAIK I can't explicitly leave a field undefined using any outer constructor, right? I suppose another solution for me would be to allow "setting" something as |
The existence of
@kwdefpredates the existence of required keyword arguments; before that, all keyword arguments needed a default value. So currently when no value is specified for a field in the type definition given to@kwdef, it's assigned a default value based on the field's type.Now, with this change, any field not explicitly given a default becomes a required keyword argument in the resulting type constructor when using
@kwdef. In doing this, we're also able to drop the requirement that the type's fields be typed, and that the type has fields at all. That is, we can now allowas well as
(though of course the latter case isn't particularly useful).
Lastly, this adds type assertions to the keyword arguments in the resulting constructor.That piece has been removed.Fixes #27970.