Skip to content

Commit

Permalink
fix: sort params order without name
Browse files Browse the repository at this point in the history 
issue: #89
  • Loading branch information
@hosseinmd
hosseinmd committed Feb 13, 2021
1 parent b17e982 commit 5337018
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 2 deletions.
9 changes: 7 additions & 2 deletions src/parser.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,13 @@ export const getParser = (parser: Parser["parse"]) =>
a.tag === PARAM &&
b.tag === PARAM
) {
//sort params
return paramsOrder.indexOf(a.name) - paramsOrder.indexOf(b.name);
const aIndex = paramsOrder.indexOf(a.name);
const bIndex = paramsOrder.indexOf(b.name);
if (aIndex > -1 && bIndex > -1) {

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 13, 2021

Contributor

@hosseinmd Because of this if, the compare function does not define a total order. This means that the output of the sort function is not defined.

Put simply, you just got lucky to get the correct result in your test case.

This solution does not work.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 13, 2021

Author Owner

We cannot sort destructed params I just ignored them.
Other tests passed which mean work

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 13, 2021

Contributor

Other tests passed

Of course they all pass. None of the existing tests had unknown parameters. There is only one test that covers this feature. As I said, you just got lucky. This solution doesn't work.

We cannot sort destructed params I just ignored them.

Exactly, we cannot sort them. So what does "ignore" mean? There is no "ignoring" in a sorting function.

Returning 0 means that 2 elements are equivalent in the ordering the compare function defines. This doesn't mean that they are ignored. It means that the relative order of the two given elements does not change. But this only works if the compare function defines a total order. Your compare function does not.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 13, 2021

Author Owner

Please add test about

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 13, 2021

Author Owner

We can just sort part of params like this, user should manually fix unknown params

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 13, 2021

Author Owner

Yes, (ignore) mean we don't move them.

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 13, 2021 •
edited

Contributor

Yes, (ignore) mean we don't move them.

There is no "don't move". All positions are relative. Only the relative position will stay the same. But, again, only if the compare function implements a total order.

We can just sort part of params like this, user should manually fix unknown params

There is no guarantee that we won't undo their fixing.

Please add test about

I didn't implement this and I have no intention of adding failing tests.

Let me ask this. What will the current implementation transform this into:

/**
 * @param something_1
 * @param c
 * @param something_2
 * @param b
 * @param something_3
 * @param a
 * @param something_4
 */
function whatever(a,b,c) {}

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 13, 2021

Author Owner

Currently, for this example we don't change anything's. But this is not a correct example because there is no params with something name

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 13, 2021 •
edited

Author Owner

However, Developer should manually remove useless params. We could not fix that

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 13, 2021 •
edited

Author Owner

I didn't implement this and I have no intention of adding failing tests.

For failing test we should find a way to add them to tests for fixing them in future.

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 14, 2021

Contributor

this is not a correct example because there is no params with something name

This is just an example. From the perspective of your code, there is no difference between my example and your test case. Some parameters are known, others are not.

Currently, for this example we don't change anything's

But why? When comparing param a and c, we clearly instruct the sort algorithm to swap their order.

Then there is also this example:

Before:

/**
 * @param something_1
 * @param c
 * @param b
 * @param something_2
 * @param something_3
 * @param a
 * @param something_4
 */
function whatever(a,b,c) {}

After:

/**
 * @param something_1
 * @param a
 * @param b
 * @param c
 * @param something_2
 * @param something_3
 * @param something_4
 */
function whatever(a, b, c) {}

Even though a and something_3 return 0, their order got swapped. Is this correct? (A: No.)

All of these problems appear because your compare function does not define a total order as required by the ECMAScript specification for Array.propotype.sort. Your function violates the transitivity requirement.

If a =CF b and b =CF c, then a =CF c (transitivity of =CF)

And as per spec:

The sort order is implementation-defined if any of the following conditions is true:

  • If comparefn is not undefined and is not a consistent comparison function for the elements of items (see below).

Your compare function is "not a consistent comparison function". Hence the output of sort is implementation-defined. This how the spec defines it:

An implementation-defined facility is one that defers its definition to an external source without further qualification. This specification does not make any recommendations for particular behaviours, and conforming implementations are free to choose any behaviour within the constraints put forth by this specification.

In the context of sorting, this means that sort may output the items in any order that it wants.

If v8 switches to a more efficient sorting algorithm or you try to run Prettier in a non-Chromium-based browser, they will likely return the items in a different order. As it is right now, you are at the mercy of v8. If they ever make any changes to their sorting algorithm, this is going to stop working.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 14, 2021

Author Owner

I couldn't found a solution yet.

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 14, 2021

Contributor

If there are unknown parameters, then don't sort parameters. That would be the easiest solution.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 14, 2021

Author Owner

I did.

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 14, 2021

Contributor

No, I mean: if there are >=1 unknown parameters, then don't sort the parameters (all of them). Parameter sorting is disabled if we find an unknown parameter. That's what I mean.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 14, 2021

Author Owner

Excuse me, but I don't understand what is a problem, this could be nice if you give me an example which has a problem.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 14, 2021

Author Owner 
/**
 * @param something_1
 * @param a
 * @param b
 * @param c
 * @param something_2
 * @param something_3
 * @param something_4
 */
function whatever(a, b, c) {}

This result is totally correct

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 14, 2021 •
edited

Contributor

Correct? Not at all.

Please answer me the following questions:

  1. Do you understand why your compare function does not define a total order?
  2. What are the consequences of using a compare function that doesn't define a total order.

I don't mean to be condescending or mean with this. I just want to make sure we are on the same page.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 14, 2021

Author Owner

total order?

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 14, 2021

Contributor

total order?

The Wikipedia article will probably explain it better than I could. There is a Persian version as well, though it seems to be rather short.

Basically, a total order is one specific way to sort a collection of things. It's a way to compare things.
Example: We could sort a list of people by age or by height. Sorting by age is a total order and sorting by height is a total order.

One of the most important properties of total orders is transitivity: If a <= b && b <= c then a <= c. (This is the property we use when doing binary search.)

To sort a list of elements, we have to pass a total order to the sorting algorithm. (We could sort a list of people by age or by height. The sorting algorithm doesn't know which order we want, so we have to tell it by providing a total order.)

In JavaScript, we do this by passing a compare function to Array.prototype.sort. The compare function takes two elements a and b and returns whether a<b, a==b, or a>b. The compare function defines the total order.

Everything clear so far?

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 15, 2021

Author Owner

We have a condition in sort we're sorting params this way, previously we always return zero for params. Now we're returning zero for unknown.
I don't think this could be a problem.
However, this is working, Until we don't have a problem. We don't change it.

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 15, 2021

Contributor

I don't think this could be a problem.

It is a problem because your compare function violates transitivity. Transitivity also implies that: if a==b && b==c then a==c. This is not true for you compare function.

/**
 * @param something_1
 * @param c
 * @param b
 * @param something_2
 * @param something_3
 * @param a
 * @param something_4
 */
function whatever(a, b, c) {}
// comp is your compare function
comp(@param a, @param something_2) -> 0
comp(@param something_2, @param b) -> 0
comp(@param a, @param b) -> -1 // or some other negative number

However, this is working

Is it? Let's look at the above example:

/**
 * @param something_1
 * @param a
 * @param b
 * @param c
 * @param something_2
 * @param something_3
 * @param something_4
 */
function whatever(a, b, c) {}

This result is totally correct

Please explain to me why the program outputs the parameters in this exact order and no other order.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 15, 2021

Author Owner

Is that matter?

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 15, 2021 •
edited

Author Owner

That is because a will compare with a-c , a-b , a-something_1

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 15, 2021

Contributor

Is that matter?

Does what matter? Context please.

That is because a will compare with a-c , a-b , a-something_1

That is not a sufficient explanation. What does "a-c" even mean?

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 15, 2021

Contributor

Is that matter?

Does what matter? Context please.

Ahh, did you mean "your compare function violates transitivity"?

If so, yes. Yes, it matters. You are using the API of the sort function incorrectly and that matters. As I said multiple times, the output order is not defined for your compare function. The violation of transitivity is the reason for that.

I already gave you an example of it not working, so here is another:

Before:

/**
 * @param c
 * @param b
 * @param something_2
 * @param something_3
 * @param a
 * @param something_4
 */
function whatever(a,b,c) {}

/**
 * @param something_1
 * @param c
 * @param b
 * @param something_2
 * @param something_3
 * @param a
 * @param something_4
 */
function whatever(a,b,c) {}

After:

/**
 * @param b
 * @param c
 * @param something_2
 * @param something_3
 * @param a
 * @param something_4
 */
function whatever(a, b, c) {}

/**
 * @param something_1
 * @param a
 * @param b
 * @param c
 * @param something_2
 * @param something_3
 * @param something_4
 */
function whatever(a, b, c) {}

Or is this "totally correct" as well?

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 15, 2021

Author Owner

Yes that is correct

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 15, 2021 •
edited

Author Owner

Don't worry about this. I will find a better solution for sorting unknown params in the future.

This comment has been minimized.

Sign in to view

Copy link
@hosseinmd

hosseinmd Feb 15, 2021

Author Owner

Thank you.

This comment has been minimized.

Sign in to view

Copy link
@RunDevelopment

RunDevelopment Feb 15, 2021

Contributor

Yes that is correct

You are joking, right?

I will find a better solution for sorting unknown params in the future.

Then reopen #89 or open a new one. Nobody is going to remember this.
//sort params
return aIndex - bIndex;
}
return 0;
}
return getTagOrderWeight(a.tag) - getTagOrderWeight(b.tag);
});
Expand Down
16 changes: 16 additions & 0 deletions tests/__snapshots__/main.test.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,22 @@ const fun = (param0: () => {}, param1: number, param2) => {
"
`;

exports[`param order 2`] = `
"/**
* @param {string} a - A for foo.
* @param {object} c - Options.
* @param {object} opts - Options.
* @param {string} opts.b - Option b.
* @param {string} v - Option b.
* @param {string} opt2 - Option b.
*/
const foo = (a, c, { b }, v, opt2) => {
console.log(a, v, c, b);
};
foo(\\"a\\", { b: \\"b\\" });
"
`;

exports[`since 1`] = `
"/** @since 3.16.0 */
"
Expand Down
23 changes: 23 additions & 0 deletions tests/main.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -525,4 +525,27 @@ async subDomain(subDomainAddress2,subDomainAddress) {
`);

expect(result).toMatchSnapshot();

const result2 = subject(
`
/**
* @param {object} c - Options.
* @param {string} a - A for foo.
* @param {object} opts - Options.
* @param {string} opts.b - Option b.
* @param {string} opt2 - Option b.
* @param {string} v - Option b.
*/
const foo = (a,c, { b }, v, opt2) => {
console.log(a,v,c, b);
};
foo('a', { b: 'b' });
`,
{
jsdocVerticalAlignment: true,
},
);

expect(result2).toMatchSnapshot();
});

0 comments on commit 5337018

Please sign in to comment.