chore: extract a result sorting function #737

nulltoken · 2019-11-05T11:05:34Z

src/formatters/utils/sortResults.ts

nulltoken · 2019-11-12T08:13:10Z

@P0lip Ready for review

nulltoken · 2019-11-12T08:17:29Z

src/formatters/utils/sortResults.ts

-  return [...results].sort((resultA, resultB) => {
-    const diff = resultA.range.start.line - resultB.range.start.line;
+const safeCastToString = (code: string | number | undefined): string => {
+  if (code === undefined) {


@P0lip Are there any use case where code is either undefined or a number? Wouldn't that be the case, as 5.0 is around the corner, could we make this a non nullable string?

AFAIK our own code includes code in each diagnostic result, but that doesn't necessarily need to be the case when it comes to end-user code.
Moreover, if we wanted to make it required, we'd need to make the change over here https://github.com/stoplightio/types/blob/master/src/diagnostics.ts#L63, as that's is the interface SpectralDiagnostic inherits from, or, alternatively, we could make code required over here https://github.com/stoplightio/spectral/blob/develop/src/types/spectral.ts#L42#L44.
Either way, code is rather an additional metadata, not something really required, so I'd keep it optional.

and yeah, we should not be throwing either.

and yeah, we should not be throwing either.

Makes sense. Pushed another proposal.

P0lip · 2019-11-12T15:11:59Z

src/formatters/utils/sortResults.ts

+  return code;
+};
+
+const ARTIFICIAL_ROOT = 'root';


Let's have a symbol here or something really unique. Root can be a potential source

@P0lip Fixed. However, I can't help but feeling slightly displeased with result. The toString() call isn't really self explicit with regards to the uniqueness intent.

Moreover, although quite unlikely, nothing prevents a caller from feeding source with a string containing Symbol(root).

Thoughts?

Let safeSource return a symbol or string. If there is a symbol returned, shift the item to the left (return -1).

@P0lip I went a different way. Do you like it better?

src/formatters/utils/sortResults.ts

P0lip · 2019-11-18T23:47:10Z

src/formatters/utils/sortResults.ts

-export const sortResults = (results: IRuleResult[]) => {
-  return [...results].sort((resultA, resultB) => {
-    const diff = resultA.range.start.line - resultB.range.start.line;
+const compareCode = (one: string | number | undefined, another: string | number | undefined): number => {


Suggested change

const compareCode = (one: string | number | undefined, another: string | number | undefined): number => {

const compareCode = (left: IDiagnostic['code'], right: IDiagnostic['code']): -1 | 0 | 1 => {

Not sure why, but I like left and right as names for such comparison functions. 😆
or codeA and codeB
Feel free to leave it as is though.

@P0lip Fixed

P0lip · 2019-11-18T23:49:07Z

src/formatters/utils/sortResults.ts

+  return one.localeCompare(another);
+};
+
+const normalize = (value: number): -1 | 0 | 1 => {


Suggested change

const normalize = (value: number): -1 | 0 | 1 => {

const normalize = (value: number): -1 | 0 | 1 => Math.max(-1, Math.min(1, Math.floor(value))) as -1 | 0 | 1;

Math.floor is optional, but it's meant to protect us from returning other values than integers.

@P0lip Hmmm. TBH, I do believe that this proposal is less easier to parse than the original (boring) code. I tend to avoid those one-liner smart constructs for readability purposes.

However, if you really want me to update the code, I'll do it, of course.

Nah, I don't really mind. :)

P0lip · 2019-11-18T23:53:23Z

src/formatters/utils/sortResults.ts

+    return 1;
+  }
+
+  return one.toString().localeCompare(another.toString(), undefined, { numeric: true });


Suggested change

return one.toString().localeCompare(another.toString(), undefined, { numeric: true });

return String(one).localeCompare(String(another), void 0, { numeric: true });

String is a slightly safer alternative. String.prototype.toString or Number.prototype.toString can always be monkey-patched in JS 😷

P0lip · 2019-11-18T23:57:10Z

src/formatters/__tests__/sortResults.test.ts

+      end: { line: 99, character: 99 },
+    },
+    message: '99',
+    severity: 99,


Suggested change

severity: 99,

severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant

P0lip · 2019-11-18T23:59:43Z

src/cli/services/linter/utils/deduplicateResults.ts


-export const deduplicateResults = (results: IRuleResult[]) => {
-  const seen: Dictionary<Dictionary<string, string>, symbol> = {};
+  const sorted = results.sort(resultsComparer);


I guess you don't want to mutate results.

Suggested change

const sorted = results.sort(resultsComparer);

const sorted = [...results].sort(resultsComparer);

@P0lip Dammit! Thanks for this.

P0lip

Looking good in general! Appreciate your work a lot. Thanks.

P0lip · 2019-11-19T00:00:07Z

src/cli/services/linter/utils/deduplicateResults.ts


-export const deduplicateResults = (results: IRuleResult[]) => {
-  const seen: Dictionary<Dictionary<string, string>, symbol> = {};
+  const sorted = results.sort(resultsComparer);


I suspect you don't intent to mutate results.

Suggested change

const sorted = results.sort(resultsComparer);

const sorted = [...results].sort(resultsComparer);

You might need to update some other places in the code to reflect the above change.
I bet certain tests will fail, i.e src/cli/services/__tests__/linter.test.ts

I bet certain tests will fail, i.e src/cli/services/tests/linter.test.ts

I was expecting them to also fail as well, but the CI server doesn't complain yet. It's possible the current harness doesn't contain enough different broken rules that hit the same line and char range for the "wrongly ordered" results to appear.

P0lip · 2019-11-19T00:01:36Z

src/cli/services/linter/utils/deduplicateResults.ts


-const ARTIFICIAL_ROOT = Symbol('root');
+  const howMany = results.length;


Suggested change

const howMany = results.length;

const totalResults = results.length;

sounds a tiny bit better to me, but I'm terrible at naming.

P0lip · 2019-11-19T00:02:15Z

src/formatters/utils/sortResults.ts


-    return diff;
-  });
+export const resultsComparer = (resultA: IRuleResult, resultB: IRuleResult): -1 | 0 | 1 => {


Now that I think of it, I believe we could name it in the way other functions are named in this file.
compareResults What do you think?

P0lip · 2019-11-19T00:04:57Z

src/cli/services/linter/utils/deduplicateResults.ts

+    filtered.push(sorted[i]);
+  }
+
+  if (resultsComparer(sorted[howMany - 1], filtered[filtered.length - 1]) !== 0) {


Technically there is a possibility of out of bounds access here (in case of filtered array).
Let's avoid it.

Suggested change

if (resultsComparer(sorted[howMany - 1], filtered[filtered.length - 1]) !== 0) {

if (filtered.length > 0 && resultsComparer(sorted[howMany - 1], filtered[filtered.length - 1]) !== 0) {

Hmmm. It's possible I'm lacking a test case here. Let me see how I can put that under the light.

@P0lip There was a bug. And thanks to you I've found it. Added a test case and changed the algo to make it backward looking (rather than forward looking). It also brings the slight benefit of making the whole thing easier to understand (and hopefully review).

P0lip · 2019-11-19T00:09:02Z

src/formatters/utils/sortResults.ts

-    return diff;
-  });
+export const resultsComparer = (resultA: IRuleResult, resultB: IRuleResult): -1 | 0 | 1 => {
+  const diffSource = compareSource(resultA.source, resultB.source);


Also, how do you feel about having a sort of loop here?
The performance difference will be negliable, but the code will look nicer, IMHO.

return normalize([ compareSource(resultA.source, resultB.source), resultA.range.start.line - resultB.range.start.line, resultA.range.start.character - resultB.range.start.character, compareCode(resultA.code, resultB.code), resultA.path.join().localeCompare(resultB.path.join()), ].find(diff => diff !== 0) ?? 0); // ?? 0 just in case all previous calls returned 0

Didn't test it at all, but it should work.

@P0lip That should work. But I feel a bit wary about this kind of code. It's usually very painful to debug/troubleshoot when it doesn't behave as expected.

For some reasons that reminds me of the jest exceptArrayContaining/ObjectContaining pattern. This is really elegant when you read it, but turns into a gigantic mess when the expectations have changed and you're trying to find what values you forgot to update in the test cases.

As usual, if you feel more at ease with this construct, I'll update the code to meet your expectations.

P0lip · 2019-11-19T00:09:29Z

src/formatters/utils/sortResults.ts

+  return one.toString().localeCompare(another.toString(), undefined, { numeric: true });
+};
+
+const compareSource = (one: string | undefined, another: string | undefined): number => {


nulltoken · 2019-11-19T08:50:24Z

@P0lip Thanks for this very thorough review. I think I've dealt with (or discussed) every comment.

Fixup commits have been pushed to make it easier for you to see the introduced changes.

Thoughts?

P0lip

The very 2 last comments on tests.
Besides these, it's good to go

P0lip · 2019-11-19T20:50:59Z

src/formatters/__tests__/sortResults.test.ts

+  });
+});
+
+describe('resultsComparer', () => {


Suggested change

describe('resultsComparer', () => {

describe('compareResults', () => {

P0lip · 2019-11-19T20:53:50Z

src/formatters/__tests__/sortResults.test.ts

+      temp = [...shuffled];
+
+      shuffled = temp.sort((_a, _b) => {
+        return Math.random() * 10 - 5;


I get the idea, but in general tests should be deterministic, so could you execute that and paste the output into some JSON file and load it here in tests?
Alternatively, you could provide an array of indices:

const indices = [-1, 0, 1, 1, -1, 0]; // etc shuffled = temp.sort((_a, _b, i) => indices[i]);

in general tests should be deterministic,

You're right. But I was really willing to keep the list of results in an sorted way to keep the test as maintainable as possible.

Your indices based proposal is a great idea that allows to do just that. 🎉 (Sadly the sort callback doesn't accept indices. I worked around that)

Fixed.

P0lip · 2019-11-19T20:54:13Z

src/formatters/__tests__/sortResults.test.ts

+      shuffled = temp.sort((_a, _b) => {
+        return Math.random() * 10 - 5;
+      });
+    } while (shuffled[0].code === results[0].code);


Is this loop actually needed?

No longer needed now that the test is deterministic.

P0lip · 2019-11-19T20:54:37Z

src/formatters/utils/sortResults.ts

+  return one.localeCompare(another);
+};
+
+const normalize = (value: number): -1 | 0 | 1 => {


Nah, I don't really mind. :)

P0lip

Thank you!

P0lip reviewed Nov 5, 2019

View reviewed changes

src/formatters/utils/sortResults.ts Outdated Show resolved Hide resolved

nulltoken changed the title ~~[WIP] spike: implement a resultComparer~~ chore: extract a result sorting function Nov 12, 2019

nulltoken commented Nov 12, 2019

View reviewed changes

P0lip reviewed Nov 12, 2019

View reviewed changes

src/formatters/utils/sortResults.ts Outdated Show resolved Hide resolved

nulltoken mentioned this pull request Nov 17, 2019

feat: drop oas2 and oas3 rulesets #773

Merged

P0lip reviewed Nov 18, 2019

View reviewed changes

P0lip reviewed Nov 19, 2019

View reviewed changes

P0lip previously approved these changes Nov 19, 2019

View reviewed changes

chore: extract a result sorting function

cad9d84

nulltoken dismissed P0lip’s stale review via cad9d84 November 20, 2019 08:56

P0lip approved these changes Nov 20, 2019

View reviewed changes

P0lip merged commit e327fcc into stoplightio:develop Nov 20, 2019

nulltoken deleted the ntk/comparer branch March 5, 2020 07:35

	const compareCode = (one: string \| number \| undefined, another: string \| number \| undefined): number => {
	const compareCode = (left: IDiagnostic['code'], right: IDiagnostic['code']): -1 \| 0 \| 1 => {

	const normalize = (value: number): -1 \| 0 \| 1 => {
	const normalize = (value: number): -1 \| 0 \| 1 => Math.max(-1, Math.min(1, Math.floor(value))) as -1 \| 0 \| 1;

	return one.toString().localeCompare(another.toString(), undefined, { numeric: true });
	return String(one).localeCompare(String(another), void 0, { numeric: true });

	severity: 99,
	severity: DiagnosticSeverity.Error, // or any other level, it's irrelevant

	const sorted = results.sort(resultsComparer);
	const sorted = [...results].sort(resultsComparer);


		const ARTIFICIAL_ROOT = Symbol('root');
		const howMany = results.length;

	const howMany = results.length;
	const totalResults = results.length;

	if (resultsComparer(sorted[howMany - 1], filtered[filtered.length - 1]) !== 0) {
	if (filtered.length > 0 && resultsComparer(sorted[howMany - 1], filtered[filtered.length - 1]) !== 0) {

	describe('resultsComparer', () => {
	describe('compareResults', () => {

chore: extract a result sorting function #737

chore: extract a result sorting function #737

Conversation

nulltoken commented Nov 5, 2019 • edited Loading

nulltoken commented Nov 12, 2019

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

P0lip Nov 18, 2019 • 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

P0lip 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

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

nulltoken Nov 19, 2019 • 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

nulltoken commented Nov 19, 2019

P0lip 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P0lip left a comment

Choose a reason for hiding this comment

nulltoken commented Nov 5, 2019 •

edited

Loading

P0lip Nov 18, 2019 •

edited

Loading

nulltoken Nov 19, 2019 •

edited

Loading