Change ConflictingField error to contain the previously loaded field #6480
Conversation
sffc
force-pushed
the
improve-conflicting-field-error
branch
from
26bdb84 to
2efc46a
Compare
| @@ -43,6 +43,45 @@ pub trait DateTimeNamesMarker: UnstableSealed { | |||
| type MetazoneLookup: NamesContainer<tz::MzPeriodV1, ()>; | |||
| } | |||
|
|
|||
| /// A trait for `Variables` that can be converted to [`ErrorField`] | |||
| pub trait MaybeAsErrorField: UnstableSealed { | |||
There was a problem hiding this comment.
issue: I think this should be Sealed not UnstableSealed: I don't see a reason for users to be able to overwrite this.
There was a problem hiding this comment.
After #6475, I think there are no more traits in datetime that don't use UnstableSealed. Why is this trait more special than all the other scaffolding traits?
That said, this type is only useful when applied to the handful of types that are used as Variable, which are fixed.
There was a problem hiding this comment.
This trait also has an invariant that resolves to a debug assert, which could be an argument that it is in a different class
There was a problem hiding this comment.
Why is this trait more special than all the other scaffolding traits?
Justification was provided for the other scaffolding traits.
As a reminder:
- GetField: implemented on datetime types, we want people to be able to provide custom ones Audit sealed traits, take 2 #6015 (comment)
- FooMarkers: I linked it to justification around custom fieldset types, and you confirmed in that thread that yo ucared about that use case.
- NamesContainer: you provided justification, that people may wish to do custom FSet combos: Audit sealed traits, take 2 #6015 (comment)
@robertbastian was of the opinion that GetField was the only one that should be exposed. We didn't discuss that disagreement in depth then, I think.
To me, consistency is an argument when user confusion is on the cards (unlikely, users aren't supposed to be looking at these), or when it's a sign for some underlying issue. The latter is possible, but I'm not really saying we have the right ontology here, just that I don't wish to expand the set for 2.0 without justification.
A valid justification here would state why a custom FooLength class would be made by a user. I can't immediately see that, I don't think it doesn't exist, but I don't wish to spend time thinking about it.
There was a problem hiding this comment.
Discussed with Shane.
Our fundamental actual disagreement ended up being that I view UnstableSealed as more restrictive on ICU4X, since to me it is a commitment that we will continue to expose the functionality even though the API may change, whereas Shane views it as "we don't know yet if we want to seal this, and unstablesealed lets us wait for others to talk to us about it".
I am not convinced of that as a principle for ICU4X overall, but I am comfortable having it be applied to datetime traits, as it's more of a matter of how a codeowner sees it.
Part of #6477