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

improvement(client): deprecation cleanup #22948

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jason-ha
Copy link
Contributor

For merge-tree and sequence packages, setup as many internal only types/values without compromising continued support for deprecated API use by customers.

For `merge-tree` and `sequence` packages, setup as many internal only types/values without compromising continued support for deprecated API use by customers.
@jason-ha jason-ha requested review from a team as code owners October 31, 2024 10:31
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: sharedstring area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API base: main PRs targeted against main branch labels Oct 31, 2024
Copy link
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

↓ packages.dds.sequence.src.intervals:
Line Coverage Change: -0.79%    Branch Coverage Change: -0.16%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 89.47% 89.31% ↓ -0.16%
Line Coverage 96.22% 95.43% ↓ -0.79%
↓ packages.dds.sequence.src:
Line Coverage Change: 0.12%    Branch Coverage Change: -0.44%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 89.47% 89.03% ↓ -0.44%
Line Coverage 89.74% 89.86% ↑ 0.12%
↓ packages.dds.merge-tree.src:
Line Coverage Change: -0.02%    Branch Coverage Change: No change
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 94.90% 94.90% → No change
Line Coverage 97.08% 97.06% ↓ -0.02%
↑ packages.dds.sequence.src.intervalIndex:
Line Coverage Change: 0.01%    Branch Coverage Change: No change
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 92.55% 92.55% → No change
Line Coverage 87.31% 87.32% ↑ 0.01%

Baseline commit: 522f76f
Baseline build: 303346
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Collaborator

Baseline CI build failed, cannot generate bundle analysis at this time


Baseline commit: 8a833a6

Generated by 🚫 dangerJS against 51c36bf

@anthony-murphy
Copy link
Contributor

Why did you create another PR which is the same as the PR i already have in progress: #22696

readonly opArgs: IMergeTreeDeltaOpArgs | undefined;
}

// @alpha (undocumented)
export const SequenceMaintenanceEvent: {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be exported

Copy link
Contributor

Choose a reason for hiding this comment

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

at a minimum you've lost the deprecation, i'm also not sure if this is technically non-breaking in all cases. i'd rather just keep the pattern i've already got

Copy link
Contributor

@anthony-murphy anthony-murphy left a comment

Choose a reason for hiding this comment

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

I'd rather go with the PR i have. i don't see any particular advantage to this approach, it just seems like a highly opinionated redo of #22696

@@ -1234,7 +1310,7 @@ export class MergeTree {
const deltaSegments: IMergeTreeSegmentDelta[] = [];
const overlappingRemoves: boolean[] = [];
pendingSegmentGroup.segments.map((pendingSegment: ISegmentLeaf) => {
const overlappingRemove = !pendingSegment.ack(pendingSegmentGroup, opArgs);
const overlappingRemove = !ackSegment(pendingSegment, pendingSegmentGroup, opArgs);
Copy link
Contributor

Choose a reason for hiding this comment

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

i didn't do this initially on purpose, as there was/is active development in the area, and i'm trying to not clobber other people's work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: sharedstring area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants