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

feat(store): M.splitArray and M.splitRecord #6597

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Conversation

erights
Copy link
Member

@erights erights commented Nov 24, 2022

As suggested at #6431 (review) , this PR is extracted from #6431 just to provide the new feature. Because it is compat, this PR does not itself refactor the usage to use the feature.

So that we only bother to change most of these error messages once, this PR also borrows some code from #6432 for generating somewhat better errors.

@erights erights self-assigned this Nov 24, 2022
@erights erights force-pushed the markm-split-reform-only branch 2 times, most recently from 9f60267 to 5fedea2 Compare November 24, 2022 02:47
@erights erights requested a review from turadg as a code owner November 24, 2022 03:09
@erights erights force-pushed the markm-split-reform-only branch from cd1cca9 to 6b8b56c Compare November 24, 2022 04:01
@erights erights force-pushed the markm-split-reform-only branch 2 times, most recently from 1371826 to 6405c35 Compare November 24, 2022 05:44
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

just to provide the new feature. Because it is compat, this PR does not itself refactor the usage to use the feature.

This PR does change about a dozen test files outside store package so it looks like there are some changes here to adopt the feature. Maybe in defendSyncArgs using the new M.splitArray. I would have expected that change to be saved for #6431 .

That's not a blocker but I am requesting the documentation updates on MatchHelper

packages/store/src/patterns/patternMatchers.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-split-reform-only branch from 6405c35 to 47b5c90 Compare November 29, 2022 13:50
@erights erights requested a review from turadg November 29, 2022 13:58
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

I see request was satisfied by a "fix" commit. That shouldn't go into master so when merging please use "squash". Or squash locally when you fix the lint error.

packages/store/src/patterns/patternMatchers.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-split-reform-only branch from 47b5c90 to 5c2885d Compare November 29, 2022 23:29
@erights erights force-pushed the markm-split-reform-only branch from 5c2885d to 6344cff Compare November 30, 2022 09:31
@erights erights added the automerge:squash Automatically squash merge label Nov 30, 2022
@mergify mergify bot merged commit e7427e3 into master Nov 30, 2022
@mergify mergify bot deleted the markm-split-reform-only branch November 30, 2022 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants