Moving fallback options into icu_provider
#3651
Conversation
robertbastian
requested review from
a team,
Manishearth,
sffc,
dminor,
zbraniecki and
nciric
as code owners
robertbastian
requested review from
Manishearth
and removed request for
dminor,
zbraniecki,
nciric and
Manishearth
There was a problem hiding this comment.
Not convinced that icu_locid is the best place; maybe @zbraniecki has thoughts.
I'd not necessarily be opposed to moving more of these primitives to icu_provider.
|
Another option I'd be happy with is to define these as |
I'm open to that, but can we keep the DataKey::fallback_config function in that world? |
| // This is a reexport of a hidden module, which works fine in HTML doc but | ||
| // doesn't seem to be reachable anywhere in JSON. |
There was a problem hiding this comment.
File an issue about this upstream somewhere?
CC @Manishearth
There was a problem hiding this comment.
I don't know if this is actually an upstream issue and I don't want to spend a day figuring it out.
There was a problem hiding this comment.
Yeah doc(hidden) is a mess, I would still recommend filing a bug with a non-reduced testcase, or mentioning it in zulip.
There was a problem hiding this comment.
This might have been rust-lang/rust#109697
robertbastian
changed the title
icu_locidicu_provider
There was a problem hiding this comment.
By the looks of that thread, they want it such that if an item is doc(hidden), it should always be doc(hidden), and it's considered a bug if a re-export of that item shows up in docs
This whole defining of enums in one crate and exporting them from some other crate is really not great anyway.
No, that thread is a rejected proposal (I was one of the people opposing it); I was just giving context on doc(hidden) having a lot of subtle behaviors that they wish to improve. That thread is a heavy-handed attempt to fix rust-lang/rust#109449 and rust-lang/rust#53417 which it links to, and there's also some discussion of JSON near the end. Those issues were fixed by adding documentation. What I'm highlighting is that that there's a bunch of subtle behavior aroudn this and some of it is unintuitive but intentional, while other bits are unintuitive and unintentional, but it's known that this area is a mess, and I think a bug report would be helpful (because it is almost certainly a bug). |
|
The thread says fairly explicitly:
|
|
Ah yes, that is the status quo. But not quite "should always be |
|
@sffc what do you want to discuss here? |
|
We have constraints that seem incompatible:
So I put it on the discussion queue in order to maybe figure out a path forward. |
I don't think that's the case |
|
Conclusion: OK to depend on the current behavior of doc(hidden); @Manishearth plans to advocate that if that behavior changes, then doc(visible) needs to be added at the same time, and we can adopt that annotation when available. |
|
@sffc please review or at least remove the request changes. This PR has multiple conflicts a day, and we decided last week that we're going to merge this as-is. |
The
LocaleFallbackConfigis the options bag for anicu::locid_transform::LocaleFallbacker. It was previously defined inicu_provider_adapters, with its membersFallbackPriorityandFallbackSupplementinicu_provider. After moving fallback logic out of provider code, they should also be moved out. However, asicu_providerdepends on them (for data key metadata), they have to be inicu_locidinstead oficu_locid_transform. I think that's an ok solution, as they define behaviour of locales.#3504