improvement(merge-tree): Use recommended eslint config #21684

jzaffiro · 2024-06-27T23:04:17Z

Updates the eslint config from minimal-deprecated to recommended, and addresses the resulting violations.

jzaffiro · 2024-06-28T23:52:00Z

packages/dds/merge-tree/api-report/merge-tree.legacy.alpha.api.md

@@ -77,7 +77,7 @@ export abstract class BaseSegment implements ISegment {
    // (undocumented)
    splitAt(pos: number): ISegment | undefined;
    // (undocumented)
-    abstract toJSONObject(): any;
+    abstract toJSONObject(): unknown;


Not sure if this is better than suppressing the errors for any json stuff...

If it affects the API surface in a way that might break consumers, we should suppress the error. This is a case where we would need to put it back to any.

We should fix other violations where they can be fixed without too much effort, but the primary goal is to prevent new violations of our rules. So feel free to disable rules anywhere it would break the API, or if it would require substantial refactoring to accommodate the changes. We should leave TODO comments in any places where we disable though.

+1 to commenting wherever we disable so we don't lose the context and don't normalize suppression.

jzaffiro · 2024-06-28T23:56:01Z

packages/dds/merge-tree/src/test/resetPendingSegmentsToOp.spec.ts

@@ -3,9 +3,10 @@
 * Licensed under the MIT License.
 */

+/* eslint-disable unicorn/no-useless-spread */


This rule has some false positives - we actually do need the spread operator, since the result of the mapping made by the autofix is an Iterable, not an array.
sindresorhus/eslint-plugin-unicorn#2018

Let's leave a comment in the code explaining this next to the disable comment.

We can do the same thing without the spread operator.
const pending = [...client.mergeTree.pendingSegments.map((n) => n.data)];
can be changed to
const pending = Array.from(client.mergeTree.pendingSegments.map((n) => n.data));

Or, since all we do is shift the array, we could replace that with just iterating and skip the array entirely?

const pending = client.mergeTree.pendingSegments.map((n) => n.data); const pendingIterator: Iterator<SegmentGroup, undefined> = pending[Symbol.iterator](); opList = oldops.map((op) => ({ op: client.regeneratePendingOp(op.op, pendingIterator.next().value!), refSeq: client.getCurrentSeq(), }));

The Array.from with 2 arguments seems good. I think that's a bit different from what I suggested to Jillian while she was looking at it, but a small variant on it was rejected by a 'prefer-spread' type rule.

FWIW, I don't really like having a rule in our recommended config with such a large gap like this. It has an autofixer which will make correct code incorrect (which was exactly what happened to Jillian here), presumably because it assumes the return of .map is already an array without actually having typing knowledge.

In most such cases it probably complains at build time in which case it's not the worst, but I'm not sure that's always the case. IIRC @taylorsw04 or @noencke mentioned at some point getting toasted by linter autofixes in id-compressor that I think was also related to spread operators, but not sure if it was this rule or not.

FWIW, I don't really like having a rule in our recommended config with such a large gap like this. It has an autofixer which will make correct code incorrect (which was exactly what happened to Jillian here), presumably because it assumes the return of .map is already an array without actually having typing knowledge.

That seems like a good reason to disable the rule, yeah. Definitely feel free to disable in the config here for this PR, since it's clearly causing problems. I can get a PR up disabling it in our shared config.

The Array.from with 2 arguments seems good. I think that's a bit different from what I suggested to Jillian while she was looking at it, but a small variant on it was rejected by a 'prefer-spread' type rule.

@Abe27342 @titrindl The Array.from is still getting rejected, so I'll leave the spread operator.

PR disabling the rule in the base config: #21804

packages/dds/merge-tree/src/attributionCollection.ts

packages/dds/merge-tree/src/attributionPolicy.ts

packages/dds/merge-tree/src/localReference.ts

Josmithr · 2024-07-01T18:27:09Z

packages/dds/merge-tree/src/localReference.ts

@@ -262,9 +271,13 @@ export class LocalReferenceCollection {
 	}

 	/**
+	 * Returns an iterator.


Can we be more specific here? What is the returned iterator intended to iterate?

Josmithr · 2024-07-01T18:32:00Z

packages/dds/merge-tree/src/mergeTree.ts

@@ -1170,17 +1205,19 @@ export class MergeTree {
 					const locallyMovedSegments = this.locallyMovedSegments.get(localMovedSeq);

 					if (locallyMovedSegments) {
-						locallyMovedSegments.segments.forEach((segment: ISegmentLeaf) => {
-							segment.localMovedSeq = undefined;
+						for (const segment of locallyMovedSegments.segments) {


The typing difference might be a good reason to revert this particular change and disable the lint rule (with a comment explaining why).

packages/dds/merge-tree/src/mergeTree.ts

Josmithr · 2024-07-09T20:42:09Z

packages/dds/merge-tree/src/mergeTreeNodes.ts

 		if (this.properties) {
 			jseg.props = { ...this.properties };
 		}
 	}

+	// eslint-disable-next-line @typescript-eslint/no-explicit-any


Let's document why we can't convert this yet and/or leave a TODO to come back later and clean this up.

Josmithr · 2024-07-09T20:43:55Z

packages/dds/merge-tree/src/mergeTreeNodes.ts

 * @internal
 */
-export const compareNumbers = (a: number, b: number) => a - b;
+export const compareNumbers = (a: number, b: number): number => a - b;


(Not for this PR) I'm surprised we need to export these from the package, even if just @internal. Curious if anyone knows why these are currently being shared.

It looks like they're used in partialLengths and a few tests, but I'm not sure if there's a specific reason to export them from here over defining them when needed. @Abe27342 thoughts?

it does look like they are exported from the package, so the tag does nothing

@anthony-murphy should it be removed then? I'm tracking some other cleanup with AB#8760, so I can do it as part of that work if so.

at minimum, the tag should be removed :-). We could also consider inlining them in the test cases we use them in if they're not strictly related (the comparisons are the idiomatic ones), but i'm kinda indifferent there.

whatever you do here, follow-up pr is obviously fine. pre-existing.

Josmithr · 2024-07-09T20:44:58Z

packages/dds/merge-tree/src/ops.ts

@@ -121,6 +121,7 @@ export interface IMergeTreeInsertMsg extends IMergeTreeDelta {
 	relativePos1?: IRelativePosition;
 	pos2?: number;
 	relativePos2?: IRelativePosition;
+	// eslint-disable-next-line @typescript-eslint/no-explicit-any


Can we leave explanations/TODOs for these disables?

Josmithr · 2024-07-09T20:46:05Z

packages/dds/merge-tree/src/ordinal.ts

 	assert(
 		maxCount <= 16 && actualCount <= maxCount,
 		0x3f0 /* count must be less than max, and max must be 16 or less */,
 	);

 	const ordinalWidth = 1 << (maxCount - actualCount);
 	let ordinal: string;
-	if (previousOrdinal === undefined) {
-		ordinal = parentOrdinal + String.fromCharCode(ordinalWidth - 1);
+	if (previousOrdinal === undefined || previousOrdinal === "") {


This appears to a behavioral change. Is this intentional / safe to make?

This is intentional - to find the parent ordinal, the tests use fromCodePoint, which returns an empty string if the result of making the elements passed in into a string has a length of 0. fromCharCode returned undefined, but the tests were not accurately determining if there was a parentOrdinal without the extra check.

gives me a minor amount of pause toward actually following this lint rule as per other comment. I'm pretty confident our tests exercise the cases we care about here though, so not a huge deal.

Josmithr · 2024-07-09T20:47:13Z

packages/dds/merge-tree/src/partialLengths.ts

@@ -381,6 +381,8 @@ export class PartialSequenceLengths {
 	}

 	/**
+	 * PartialLengths for the leaves of a given MergeBlock.


Nit: This isn't clear to me. Might be helpful to write this out as a complete sentence for clarity.

packages/dds/merge-tree/src/properties.ts

packages/dds/merge-tree/src/test/client.localReference.spec.ts

packages/dds/merge-tree/src/test/dirname.cts

Josmithr · 2024-07-09T20:57:34Z

packages/dds/merge-tree/src/test/obliterate.partialLength.spec.ts

@@ -140,7 +140,7 @@ describe("obliterate partial lengths", () => {
 				refSeq,
 			);
 		});
-
+		// here are tests failing w nouncheckedindexaccess


Was this intentional?

packages/dds/merge-tree/src/test/sortedSegmentSet.spec.ts

Josmithr

I left a few more suggestions / nitpicks, but overall looks good. Thanks for working through all of this - this was one of the biggest packages left to migrate!

Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>

Abe27342 · 2024-07-11T21:29:11Z

packages/dds/merge-tree/src/attributionCollection.ts

@@ -320,6 +325,7 @@ export class AttributionCollection implements IAttributionCollection<Attribution
 			if (i !== 0 || !areEqualAttributionKeys(lastEntry, other.keys[i])) {
 				// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
 				this.offsets.push(other.offsets[i]! + this.length);
+				// TODO: see line 305


@jzaffiro, just calling your attention here - was merged like this

packages/dds/merge-tree/src/attributionPolicy.ts

Abe27342 · 2024-07-11T21:46:38Z

packages/dds/merge-tree/src/endOfTreeSegment.ts

 		// just compute an arbitrarily big ordinal
 		// we base it on the depth of the tree
 		// to ensure it is bigger than all ordinals in
 		// the tree, as each layer appends to the previous
-		return String.fromCharCode(0xffff).repeat(this.endpointSegmentProps().depth);
+		return String.fromCodePoint(0xffff).repeat(this.endpointSegmentProps().depth);


though it's not a huge deal and I think the behavior here is correct, I would have leaned toward disabling the lint rule when dealing with ordinals. The intent of this lint rule is probably "you probably don't support unicode correctly if you're using this and likely have bugs", but for ordinals we obviously aren't actually ever dealing with user input, we're just using it for dictionary order.

i think it's fine as is, but worth a comment. whatever you go with, just be consistent that all logic related to ordinals uses a consistent method.

packages/dds/merge-tree/src/partialLengths.ts

Abe27342 · 2024-07-11T22:11:38Z

packages/dds/merge-tree/src/mergeTree.ts

+	return siblingExists;
+};
+
+const onLeaf = (


nit: I would not remove onLeaf and continueFrom from their scope. this makes the code harder to read than before, even if it is technically not necessary to create a new function on each execution.

if they had informative names perhaps it'd be ok as well.

Abe27342 · 2024-07-11T22:21:20Z

feel free to address all my comments in a follow-up PR. I don't really want to look at this again and it'll be easier to audit that way anyway.

Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

Minor improvements to #21684, including disabling [prefer-code-point](https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-code-point.md) for ordinals, reverting function scope changes, and updating documentation. --------- Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

Updates the eslint config from minimal-deprecated to recommended, and addresses the resulting violations. [AB#3014](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/3014) --------- Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com> Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

Minor improvements to microsoft#21684, including disabling [prefer-code-point](https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-code-point.md) for ordinals, reverting function scope changes, and updating documentation. --------- Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

jzaffiro added 6 commits June 17, 2024 21:34

WIP: upgrade eslint config

b7a53af

WIP: upgrade eslint config

6f82700

WIP: upgrade eslint config

2889e84

WIP: upgrade lint config

fca6758

Add suppressions for intentional anys

4a3b8f8

WIP: upgrade eslint config

8de4974

github-actions bot added area: dds Issues related to distributed data structures base: main PRs targeted against main branch labels Jun 27, 2024

jzaffiro added 6 commits June 27, 2024 23:49

Address violations + prettier

34ec4be

Merge branch 'main' into updateMergeTreeConfig

7479f0e

biome formatting

537f64d

Autofix violations

25e0a55

Test farm suppressions

26b401f

Address remaining violations

f3ae90f

github-actions bot added the public api change Changes to a public API label Jun 28, 2024

jzaffiro marked this pull request as ready for review June 28, 2024 23:51

jzaffiro requested review from a team as code owners June 28, 2024 23:51

jzaffiro requested review from Abe27342 and Josmithr June 28, 2024 23:51

jzaffiro commented Jun 28, 2024

View reviewed changes

Fix formatting

9c9ef13

jzaffiro commented Jun 28, 2024

View reviewed changes

Josmithr reviewed Jun 29, 2024

View reviewed changes

packages/dds/merge-tree/src/attributionCollection.ts Show resolved Hide resolved

Josmithr reviewed Jun 29, 2024

View reviewed changes

packages/dds/merge-tree/src/attributionPolicy.ts Outdated Show resolved Hide resolved

Josmithr reviewed Jun 29, 2024

View reviewed changes

packages/dds/merge-tree/src/attributionPolicy.ts Outdated Show resolved Hide resolved

Josmithr reviewed Jul 1, 2024

View reviewed changes

packages/dds/merge-tree/src/localReference.ts Outdated Show resolved Hide resolved

Josmithr reviewed Jul 1, 2024

View reviewed changes

packages/dds/merge-tree/src/mergeTree.ts Show resolved Hide resolved

Josmithr reviewed Jul 9, 2024

View reviewed changes

packages/dds/merge-tree/src/properties.ts Show resolved Hide resolved

Josmithr reviewed Jul 9, 2024

View reviewed changes

packages/dds/merge-tree/src/test/client.localReference.spec.ts Outdated Show resolved Hide resolved

Josmithr reviewed Jul 9, 2024

View reviewed changes

packages/dds/merge-tree/src/test/dirname.cts Show resolved Hide resolved

Josmithr reviewed Jul 9, 2024

View reviewed changes

packages/dds/merge-tree/src/test/sortedSegmentSet.spec.ts Show resolved Hide resolved

Josmithr approved these changes Jul 9, 2024

View reviewed changes

jzaffiro and others added 2 commits July 9, 2024 15:03

Apply suggestions from code review

d6b0091

Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>

Add changeset and address other pr feedback

4bea1e5

tylerbutler changed the title ~~Update merge tree config~~ Jul 10, 2024

tylerbutler changed the title ~~improvement(merge-tree): Use strict eslint config~~ Jul 10, 2024

Abe27342 reviewed Jul 11, 2024

View reviewed changes

packages/dds/merge-tree/src/attributionPolicy.ts Outdated Show resolved Hide resolved

Abe27342 reviewed Jul 11, 2024

View reviewed changes

packages/dds/merge-tree/src/partialLengths.ts Outdated Show resolved Hide resolved

Abe27342 reviewed Jul 11, 2024

View reviewed changes

Abe27342 approved these changes Jul 11, 2024

View reviewed changes

Apply suggestions from code review

67b603c

Co-authored-by: Abram Sanderson <Abram.sanderson@gmail.com>

jzaffiro enabled auto-merge (squash) July 11, 2024 22:33

jzaffiro merged commit d2d472b into microsoft:main Jul 11, 2024
30 checks passed

jzaffiro mentioned this pull request Jul 12, 2024

chore(merge-tree): Cleanup related to lint fixes #21870

Merged

jzaffiro deleted the updateMergeTreeConfig branch July 12, 2024 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement(merge-tree): Use recommended eslint config #21684

improvement(merge-tree): Use recommended eslint config #21684

jzaffiro commented Jun 27, 2024 •

edited by azure-boards bot

Loading

jzaffiro Jun 28, 2024

Josmithr Jun 29, 2024 •

edited

Loading

scarlettjlee Jun 29, 2024

jzaffiro Jun 28, 2024

Josmithr Jun 29, 2024

titrindl Jun 29, 2024

titrindl Jun 29, 2024

Abe27342 Jul 1, 2024

Abe27342 Jul 1, 2024

Josmithr Jul 1, 2024

jzaffiro Jul 5, 2024 •

edited

Loading

Josmithr Jul 9, 2024

Josmithr Jul 1, 2024

Josmithr Jul 1, 2024

Josmithr Jul 9, 2024

Josmithr Jul 9, 2024

jzaffiro Jul 10, 2024

anthony-murphy Jul 11, 2024

jzaffiro Jul 11, 2024

Abe27342 Jul 11, 2024

Abe27342 Jul 11, 2024

Josmithr Jul 9, 2024

Josmithr Jul 9, 2024

jzaffiro Jul 9, 2024

Abe27342 Jul 11, 2024

Josmithr Jul 9, 2024

Josmithr Jul 9, 2024

Josmithr left a comment

Abe27342 Jul 11, 2024

jason-ha Jul 12, 2024

Abe27342 Jul 11, 2024

Abe27342 Jul 11, 2024

Abe27342 Jul 11, 2024

Abe27342 commented Jul 11, 2024

improvement(merge-tree): Use recommended eslint config #21684

improvement(merge-tree): Use recommended eslint config #21684

Conversation

jzaffiro commented Jun 27, 2024 • edited by azure-boards bot Loading

Choose a reason for hiding this comment

Josmithr Jun 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzaffiro Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josmithr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abe27342 commented Jul 11, 2024

jzaffiro commented Jun 27, 2024 •

edited by azure-boards bot

Loading

Josmithr Jun 29, 2024 •

edited

Loading

jzaffiro Jul 5, 2024 •

edited

Loading