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

WIP: Changeset: Refactor to improve API, and other cleanups #5233

Draft
wants to merge 19 commits into
base: develop
Choose a base branch
from
Draft

WIP: Changeset: Refactor to improve API, and other cleanups #5233

wants to merge 19 commits into from

Conversation

rhansen
Copy link
Member

@rhansen rhansen commented Oct 17, 2021 •
edited

DO NOT MERGE: This will be split into multiple smaller pull requests to make things easier to review.

The code in Changeset.js had lots of readability problems so I spent some time trying to clean it up. There shouldn't be any behavior change.

cc @webzwo0i

@rhansen rhansen force-pushed the rhansen-changeset branch 2 times, most recently from 3e7389a to 68d5830 Compare October 19, 2021 08:38
@rhansen rhansen marked this pull request as draft October 21, 2021 02:53
@rhansen rhansen force-pushed the rhansen-changeset branch 3 times, most recently from eba354f to 8c83307 Compare October 30, 2021 19:23
@webzwo0i webzwo0i mentioned this pull request Nov 9, 2021
Open
@webzwo0i
Copy link
Member

webzwo0i commented Nov 9, 2021

Sorry for the late response! This is great work.

Should I cherry-pick some of the commits into new PRs or should I write my comments here? Might get messy fast...

I think having #5267 would be really useful here

@rhansen
Copy link
Member Author

rhansen commented Nov 10, 2021 •
edited

@webzwo0i I'm going to split this PR into multiple easier-to-digest PRs. #5268 is the first of many. My preference would be to comment on those individual PRs instead of here.

I think having #5267 would be really useful here

Agreed, though I don't think it should block this work.

@rhansen rhansen mentioned this pull request Nov 10, 2021
Merged
@rhansen rhansen changed the title Changeset: Refactor to improve API, and other cleanups WIP: Changeset: Refactor to improve API, and other cleanups Nov 10, 2021
@rhansen rhansen force-pushed the rhansen-changeset branch 3 times, most recently from cbf0873 to d7ac033 Compare November 14, 2021 09:33
@rhansen rhansen mentioned this pull request Nov 14, 2021
Merged
@rhansen rhansen force-pushed the rhansen-changeset branch 3 times, most recently from 6b7d14a to 12800f6 Compare November 22, 2021 04:03
This was referenced Nov 22, 2021
Merged
Merged
@rhansen rhansen mentioned this pull request Nov 23, 2021
Merged
@rhansen rhansen force-pushed the rhansen-changeset branch 3 times, most recently from 3c19543 to 27d0e11 Compare November 29, 2021 09:50
@rhansen
Changeset: Use clear() to initialize
04ed432 
This avoids some duplicate code and it helps ensure that `clear()`
works properly.
@rhansen
Changeset: Implement OpAssembler with a new serializeOps() function
d5a7bf7
@rhansen
Changeset: Migrate from opAssembler() to serializeOps()
b5486b6
@rhansen
Changeset: Use a generator to implement MergingOpAssembler
daa6b90 
Eventually all uses of the class will be switched to the generator.
@rhansen
Changeset: Migrate from mergingOpAssembler() to squashOps()
2448fb8
@rhansen
Changeset: Use a generator to implement SmartOpAssembler
23e7809 
Eventually all uses of the class will be switched to the generator.
@rhansen
Changeset: Migrate from smartOpAssembler() to canonicalizeOps()
d3d2090
@rhansen
Changeset: Turn builder() into a real class
a1c4382
@rhansen
Changeset: Add new Builder.prototype.build() method
29da981
@rhansen
Changeset: Make the Builder attribs and pool args optional
3fe2b17
@rhansen
Changeset: Turn stringAssembler() into a real class
2d0e393
@rhansen
Changeset: Avoid unnecessary StringAssembler class
cca906e
@rhansen
Changeset: Turn stringIterator() into a real class
a470253
@rhansen
Changeset: Move changeset logic to a new Changeset class
b718d88
@rhansen
Changeset: Deprecate oldLen() and newLen() functions
60f2a05
@stale
Copy link

stale bot commented Apr 25, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Wont Fix these things, no hate. label Apr 25, 2022
@stale stale bot closed this May 2, 2022
@rhansen rhansen reopened this May 6, 2022
@stale stale bot removed the wontfix Wont Fix these things, no hate. label May 6, 2022
@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Wont Fix these things, no hate. label Jul 10, 2022
@rhansen rhansen removed the wontfix Wont Fix these things, no hate. label Jul 16, 2022
@github-actions github-actions bot added the Stale No recent activity label Jun 23, 2023
@github-actions github-actions bot removed the Stale No recent activity label Jun 27, 2023
@github-actions github-actions bot added the Stale No recent activity label Dec 10, 2023
@github-actions github-actions bot removed the Stale No recent activity label Dec 20, 2023
@github-actions github-actions bot added the Stale No recent activity label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Stale No recent activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rhansen @webzwo0i