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

Specify custom sort algorithm #27

Closed
EliteMasterEric opened this issue Feb 18, 2021 · 6 comments · Fixed by #92
Closed

Specify custom sort algorithm #27

EliteMasterEric opened this issue Feb 18, 2021 · 6 comments · Fixed by #92
Labels
enhancement New feature or request

Comments

@EliteMasterEric
Copy link
Contributor

Currently the plugin only allows sorting JSON keys using the default sort order. (The default sort order is ascending, built upon converting the elements into strings, then comparing their sequences of UTF-16 code units values.) Ideally, this plugin would allow for the customization of the sorting algorithm used via configuration.

We could have a set of presets available, but instead for maximum flexibility I would like the user to be able to specify a function with the signature function(a: any, b: any): int as described here which will be passed as an argument to Object.keys(object).sort() here:

https://github.com/Gudahtt/prettier-plugin-sort-json/blob/main/index.ts#L13

Note that (as specified by Prettier) options can only be an int, boolean, choice, or path.

Here is my proposal:

printWidth: 90
tabWidth: 4
overrides:
  - files: "**/data/*.json"
    options:
      plugins:
        - "./node_modules/prettier-plugin-sort-json"
      jsonRecursiveSort: true
      jsonObjectSortAlgorithm: "./config/sortFunction.js"
      jsonArraySortAlgorithm: "./config/sortFunctionB.js"

./config/sortFunction.js is a JavaScript file with contents like such:

export default (a, b) => {
  if (a < b) {
    return -1;
  } else if (a > b) {
    return 1;
  }
  return 0;
};

This scheme is highly flexible; any desired sorting algorithm can be used, a custom pre-defined order can be defined simply by comparing with a constant array, and with the Prettier configuration system that already exists, different sorting algorithms could be used for different files.

@EliteMasterEric
Copy link
Contributor Author

EliteMasterEric commented Feb 19, 2021

I am in the process of developing a pull request for this, and found the following caveat.

In the rare circumstance that a user has an object with a key whose value is a number (such as { 0: true}), those numbers will ALWAYS be displayed in ascending order and ALWAYS display before string keys.

This key order is defined by JavaScript, and the only way to fix it would be to store the order of the keys during the sorting step, then override the JSON.stringify implementation to respect said order.

Since I did not want to perform a massive overhaul like this for a rare edge case, I have tried to document this caveat where relevant in the test cases.

@EliteMasterEric
Copy link
Contributor Author

Resolved by PR #30.

@Gudahtt
Copy link
Owner

Gudahtt commented Feb 22, 2021

Interesting idea 🤔 This is sorta like creating a plugin system for this plugin!

I had considered something like this, but I thought it would be difficult if not impossible to define a custom sort programmatically as an option, because of the type constraint that you pointed out.

This is an interesting workaround, though I am a bit put off by the dynamic require. We don't have any way to know what the imported module might do - it might start a server and leave it running, or do any other weird or sneaky thing, and then it'd look like this plugin was responsible for whatever it did. It's also a bit unintuitive for the user of this plugin, having to provide a path to a module? I'm not sure.

Maybe we could pursue something more like #31, but allow setting the sort order of entire classes of characters? Specifying a custom sort order without having to import anything would be ideal.

@Gudahtt Gudahtt added the enhancement New feature or request label Feb 22, 2021
@EliteMasterEric
Copy link
Contributor Author

This is an interesting workaround, though I am a bit put off by the dynamic require. We don't have any way to know what the imported module might do

My stance is that there aren't any circumstances where this would be a problem. I see the module file as basically an extension of the workspace's prettier configuration, and it's on the user to ensure that their setup makes sense. I don't see how this opens up any malicious code to be run that couldn't just happen by inserting code directly into the project.

@Gudahtt
Copy link
Owner

Gudahtt commented Aug 14, 2022

I'm still not comfortable with the dynamic require. It would give me pause before using this in my own projects. Supply chain security is complicated enough already.

I have just developed a solution using a JSON configuration file. You can see it here: #92

In the rare circumstance that a user has an object with a key whose value is a number (such as { 0: true}), those numbers will ALWAYS be displayed in ascending order and ALWAYS display before string keys.

This should no longer be a problem since I have switched to sorting the AST in #100. I'm no longer using JSON.stringify, which is what was imposing this order.

Gudahtt added a commit that referenced this issue Aug 14, 2022
An option has been added to specify a custom sort order using a JSON
file.

Closes #27 and #31
Gudahtt added a commit that referenced this issue Aug 14, 2022
An option has been added to specify a custom sort order using a JSON
file.

Closes #27, #31, and #57
Gudahtt added a commit that referenced this issue Aug 14, 2022
An option has been added to specify a custom sort order using a JSON
file.

Closes #27, #31, and #53
@Gudahtt
Copy link
Owner

Gudahtt commented Aug 14, 2022

This was implemented as part of #92. Let me know what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants