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

Add middle attributes sorting #120

Merged
merged 16 commits into from
Oct 2, 2020

Conversation

SkyaTura
Copy link
Collaborator

@SkyaTura SkyaTura commented Sep 30, 2020

TODO

  • Explain this PR

Original post

## Reason for this PR

Every time this code is saved it gets a different result:

<template lang="pug">
Foo(c b a d i f g h e l n)
</template>

Expected result

<template lang="pug">
Foo(a b c d e f g h i l n)
</template>

@SkyaTura SkyaTura marked this pull request as draft September 30, 2020 03:35
@SkyaTura
Copy link
Collaborator Author

I've accidentally formatted things that I shouldn't on a formatter library :P

Going to fix it right now!

@SkyaTura SkyaTura marked this pull request as ready for review September 30, 2020 03:40
@SkyaTura SkyaTura mentioned this pull request Sep 30, 2020
5 tasks
@Shinigami92
Copy link
Member

  1. Please use draft PRs
  2. Please request me as a reviewer if you are done

🙂

@Shinigami92
Copy link
Member

Please add a prettier config to the issue descriptions

Currently I would expect that nothing would get sorted cause the default would be taken

Also please note that the sorting isn't sorting in an alphabetical order in any way

@Shinigami92 Shinigami92 marked this pull request as draft September 30, 2020 06:13
src/options/attribute-sorting/utils.ts Outdated Show resolved Hide resolved
src/options/attribute-sorting/utils.ts Outdated Show resolved Hide resolved
@SkyaTura
Copy link
Collaborator Author

SkyaTura commented Sep 30, 2020

So, I'm thinking and experimenting things for long hours now, but I can't find an answer.

What have I done since my first changes:

  • In fact, this formatter had nothing to do with "middle attributes", so I've added a new option pugSortAttributes = 'as-is' | 'asc' | 'desc'. This could be reverted if you think it shouldn't exist.
  • Added more test cases, with different middle attribute sorting options.

What I cannot understand is, why on earth compare 3 and compare 6 are failing.

This is the main problem, which I though I was solving. I kinda only refactored some code that was already working. 🤦🏻

@Shinigami92
Copy link
Member

Shinigami92 commented Sep 30, 2020

Can you rename the PR title? :)

And also you should merge master into your PR branch

@SkyaTura SkyaTura changed the title Fix attribute sorting Add middle attributes sorting Sep 30, 2020
@Shinigami92
Copy link
Member

@SkyaTura Looking at the tests, it seems that your sort is not stable 🤔

@SkyaTura
Copy link
Collaborator Author

SkyaTura commented Oct 1, 2020

Yes, but I don't know why. My knowledge is limited here, because I though it should work correctly.

I'll study more about the sort method to understand what is going on, over this weekend, ( the last couple days have been busy for me :x )

@lehni
Copy link
Collaborator

lehni commented Oct 1, 2020

Could it be the cases where the compare function returns 0 that leads to unstable sort?

Node's sort() wasn't stable up to about Node 11 or so.

Some bits about this:

https://bugs.chromium.org/p/v8/issues/detail?id=90
nodejs/node#29446 (comment)
https://medium.com/@fsufitch/is-javascript-array-sort-stable-46b90822543f

I would try a stable sort() method next, e.g.:

function stableSort(
  array,
  compare = (a, b) => a < b ? -1 : a > b ? 1 : 0
 ) {
  const entries = array.map((value, index) => [value, index]);
  entries.sort((a, b) => {
    const order = compare(a[0], b[0]);
    // When order is 0, sort by index to make the sort stable:
    return order != 0 ? order : a[1] - b[1];
  });
  return entries.map(([value]) => value);
}

Note that this doesn't modify array, it returns a sorted copy.

@SkyaTura
Copy link
Collaborator Author

SkyaTura commented Oct 2, 2020

Finally!!! My logic wasn't wrong after all 😅

Thank you a lot @lehni . This stableSort worked as a chamr.

@Shinigami92
Copy link
Member

Shinigami92 commented Oct 2, 2020

@SkyaTura @lehni Check out my newly added contributing.md 😃
Maybe it could help 🙂

@SkyaTura SkyaTura marked this pull request as ready for review October 2, 2020 12:28
@lehni
Copy link
Collaborator

lehni commented Oct 2, 2020

@SkyaTura happy to hear that this helped! I was struggling with this situation a while back on Node also, it can be so confusing.

src/options/attribute-sorting/index.ts Outdated Show resolved Hide resolved
src/options/attribute-sorting/utils.ts Outdated Show resolved Hide resolved
src/options/attribute-sorting/utils.ts Outdated Show resolved Hide resolved
return 0;
}

export function stableSort<T>(array: T[], compare: CompareFunction): any[] {
Copy link
Member

Choose a reason for hiding this comment

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

Not quite sure, but isn't it : T[] as return type?

Copy link
Collaborator Author

@SkyaTura SkyaTura Oct 2, 2020

Choose a reason for hiding this comment

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

To be honest, I'm new to TypeScript, I didn't knew how to properly type the method @lehni suggested.

Could you help me with this one?

It took a while to understand hahah

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, I can checkout your branch at evening and then suggest some types

tests/options/sortAttributes/sort-attributes.test.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 self-requested a review October 2, 2020 15:37
@Shinigami92 Shinigami92 merged commit aac102a into prettier:master Oct 2, 2020
@Shinigami92 Shinigami92 added the type: feature request Functionality that introduces a new feature label Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request Functionality that introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants