Clarify that modifyproperty! does not call convert currently
#45218
Conversation
tkf
added
docs
base/docs/basedocs.jl
Outdated
| @@ -2758,6 +2758,11 @@ The syntax `@atomic! max(a().b, c)` returns `modifyproperty!(a(), :b, | |||
| max, c, :sequentially_consistent))`, where the first argument must be a | |||
| `getfield` expression and is modified atomically. | |||
|
|
|||
| In the current implementation, the default implementation of `modifyproperty!` does not call | |||
There was a problem hiding this comment.
Typically documentation is written in the imperative tense, and tries to avoid conditional modifiers of this sort.
There was a problem hiding this comment.
Any suggestions? How about something like
Invocation of
op(getproperty(x, f), v)must return a value that can be stored in the fieldfof the objectx. In particular, unlike the default behavior ofsetproperty!, theconvertfunction is not called automatically. However, this limitation may be removed in the future.
There was a problem hiding this comment.
Yep, that reads better. I think remove the comment about "in the future" though (we also can change errors -> working as a rule, so it is unnecessary to state it)
There was a problem hiding this comment.
we also can change errors -> working as a rule
Yeah, it would greatly simply documentation like this. But some exceptions (e.g., LinearAlgebra.SingularException, Base.IOError) are used as an "output" of a function. So, I think we need to exclude some functions if we set this as a rule. Also, to handle APIs that want to guarantee to return an error with information in certain cases, I think we need something like Union{Ok,Err} convention #45080. After that, it'd be nice to define that changing throwing to not-throwing is non-breaking but changing return value, including Union{Ok,Err}, is breaking.
tkf
force-pushed
the
modifyproperty-convert
branch
from
334ff83 to
dfe5eea
Compare
|
@vtjnash I added the commit to use the version in #45218 (comment), without the "in the future" part. |
Re-applying #45178 (which was reverted in #45209) and also clarifying that this is a limitation of the current implementation.
@vtjnash What do you think?