Skip to content
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

Move casemap out of experimental; final graduation cleanups #3803

Merged
merged 21 commits into from
Aug 9, 2023

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Aug 7, 2023

Part of #3234

There is one leftover issue to be decided there, #3801 , btu I'm comfortable doing the moving right now and we can fix that before release if needed.

@Manishearth Manishearth requested review from a team, sffc and robertbastian as code owners August 7, 2023 18:21
docs/tutorials/testing/patch.toml Outdated Show resolved Hide resolved
provider/datagen/src/registry.rs Show resolved Hide resolved
@Manishearth
Copy link
Member Author

One question is: icu_casemap the feature got removed from datagen and the metacrate, since it's mandatory now. Should I still have a placeholder feature? or for experimental stuff do we consider the features themselves to be experimental

@robertbastian
Copy link
Member

We should remove the feature, clients will need to update their code for API changes anyway.

robertbastian
robertbastian previously approved these changes Aug 7, 2023
sffc
sffc previously requested changes Aug 8, 2023
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sffc
Copy link
Member

sffc commented Aug 8, 2023

One question is: icu_casemap the feature got removed from datagen and the metacrate, since it's mandatory now.

I thought both datagen and the metacrate had component-specific features ?

@Manishearth
Copy link
Member Author

Manishearth commented Aug 8, 2023

@sffc can we please merge this without finishing the checklist? That checklist was not complete before you added more entries to it, I'm aware there's more work to be done there.

I don't think moving this component must be the last step, I'm happy to remove the "fixes ..." from the PR body but there's basically no chance 1.3 is happening without stabilizing this and we should land this somewhat churny PR at some point.

I thought both datagen and the metacrate had component-specific features ?

nope, only for experimental

this is the first time in the datagen features world we are graduating from experimental

that said, it occurs to me these features did not exist pre-1.3 anyway so this change is something that can be made without breaking anyone; next release we will need to figure out what to do about experimental components that graduate

@Manishearth Manishearth changed the title Move casemap out of experimental Move casemap out of experimental; final graduation cleanups Aug 8, 2023
@Manishearth
Copy link
Member Author

Manishearth commented Aug 8, 2023

I had some time and I've gone ahead and resolved most of the checklist items. I'd prefer to get this merged to avoid conflicts rather than blocking this on incomplete checklists, though, so if there are things still unhandled I would love to know but would prefer to not block landing on it.

@sffc
Copy link
Member

sffc commented Aug 9, 2023

nope, only for experimental

ok, it's not implemented yet: #3165

I think we've mentioned whether we want to do this for the metacrate but I think the consensus has been that you should use the specific crates instead of the metacrate in that situation.

@sffc sffc dismissed their stale review August 9, 2023 01:21

I'll defer to Robert on this; the PR is large with many commits and I don't have time to give it a thorough review

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer to Rob on this PR. If Rob doesn't review then add me back in a few days.

@Manishearth
Copy link
Member Author

autosubmit=true

@robertbastian
Copy link
Member

There are lots of files showing as additions, I think you have components/casemap/casemap

Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some changes so autosubmit turned off

@Manishearth Manishearth merged commit 1814f85 into unicode-org:main Aug 9, 2023
25 checks passed
@Manishearth Manishearth deleted the casemap-graduate branch August 9, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants