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.

Sign up for GitHub

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

Jump to bottom

refactor: Use new M.splitArray and M.splitRecord #6431

Merged
merged 1 commit into from
Dec 1, 2022
Merged

refactor: Use new M.splitArray and M.splitRecord #6431

merged 1 commit into from
Dec 1, 2022

Conversation

erights
Copy link
Member

@erights erights commented Oct 10, 2022 •
edited
Loading

Replace M.split and M.partial with M.splitArgs and M.splitRecord, where the latter two have required, optional, and rest parts.

Anticipates the compression from #6432

@erights erights self-assigned this Oct 10, 2022
@erights erights marked this pull request as ready for review October 10, 2022 08:00
@erights erights force-pushed the markm-split-reform branch 2 times, most recently from f1b5db9 to 3476d57 Compare October 10, 2022 08:41
@erights erights mentioned this pull request Oct 10, 2022
Draft
@erights erights requested a review from mhofman October 11, 2022 23:52
@erights erights mentioned this pull request Oct 19, 2022
Closed
@erights
Copy link
Member Author

erights commented Nov 5, 2022

Low priority, but may be easy. Ping.

turadg
turadg reviewed Nov 9, 2022
View reviewed changes
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.

Removing the need for M.partial is an improvement.

To make git history cleaner and reviewing easier, please consider separating the new code from the refactoring. The changes in packages/store could land in master without any others, since they're backwards compatible.

Also for commit history none of this is a "fix".
feat: splitArgs and splitRecord would describe the new functionality.
refactor: use new splitArgs and splitRecord would describe the changes to make use of them.

For the refactoring commit there's one more in casting/src/netconfig.js (added since this PR started)

packages/store/src/types.js Outdated Show resolved Hide resolved
packages/store/src/types.js Outdated Show resolved Hide resolved
@erights
Copy link
Member Author

erights commented Nov 9, 2022

To make git history cleaner and reviewing easier, please consider separating the new code from the refactoring. The changes in packages/store could land in master without any others, since they're backwards compatible.

Good suggestion! Since this splits the PR into two, I'm putting this PR back into Draft until it is split that way.

@erights erights marked this pull request as draft November 9, 2022 22:00
@erights erights force-pushed the markm-split-reform branch 2 times, most recently from b2e7f5a to 6e99025 Compare November 24, 2022 02:05
@erights erights mentioned this pull request Nov 24, 2022
Merged
@erights erights changed the base branch from master to markm-split-reform-only November 24, 2022 02:22
@erights erights changed the title fix: split reform refactor: Use new M.splitArray and M.splitRecord Nov 24, 2022
@erights erights force-pushed the markm-split-reform branch 2 times, most recently from 5d3b773 to 708baa7 Compare November 24, 2022 02:46
@erights
Copy link
Member Author

erights commented Nov 24, 2022

Removing the need for M.partial is an improvement.

To make git history cleaner and reviewing easier, please consider separating the new code from the refactoring. The changes in packages/store could land in master without any others, since they're backwards compatible.

Done. With the base extracted to #6597 , both are now Ready for Review.

Also for commit history none of this is a "fix". feat: splitArgs and splitRecord would describe the new functionality. refactor: use new splitArgs and splitRecord would describe the changes to make use of them.

Done.

For the refactoring commit there's one more in casting/src/netconfig.js (added since this PR started)

Done.

@erights erights marked this pull request as ready for review November 24, 2022 04:12
@erights erights force-pushed the markm-split-reform-only branch 3 times, most recently from 5c2885d to 6344cff Compare November 30, 2022 09:31
Base automatically changed from markm-split-reform-only to master November 30, 2022 10:11
@erights
Copy link
Member Author

erights commented Nov 30, 2022

Done. With the base extracted to #6597 , both are now Ready for Review.

#6597 is now merged, so this PR is now a candidate for merging to master.

@erights erights requested a review from turadg November 30, 2022 11:09
turadg
turadg approved these changes Nov 30, 2022
View reviewed changes
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.

LGTM. Thanks for splitting.

packages/SwingSet/src/typeGuards.js Outdated Show resolved Hide resolved
@erights erights added the automerge:squash Automatically squash merge label Dec 1, 2022
@mergify mergify bot merged commit 7844f69 into master Dec 1, 2022
@mergify mergify bot deleted the markm-split-reform branch December 1, 2022 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turadg turadg turadg approved these changes

@dtribble dtribble Awaiting requested review from dtribble

@gibson042 gibson042 Awaiting requested review from gibson042

@mhofman mhofman Awaiting requested review from mhofman

Assignees

@erights erights

Labels
automerge:squash Automatically squash merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erights @turadg