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

Added option "sortingOrder" #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ccencisoj
Copy link

Hello Sander,

I have created this pull request to add the "sortingOrder" option. This option allows defining whether the order of imports should be in "ascending" or "descending" order.

For example:

In "ascending" order:
import a from 'a'
import ab from 'ab';
import abc from 'abc';

In "descending" order:
import abc from 'abc';
import ab from 'ab';
import a from 'a';

This option only applies when "sortingMethod" is defined as "lineLength".

I have tested the changes locally and I am confident that they work correctly. I have also ensured that all tests pass without any issues.

I would love to receive your feedback on the code and any suggestions you may have to improve it. In particular, if you have any ideas on how to improve code readability, I would be interested in hearing them.

Best regards,
Cristian Enciso

category: 'Global',
type: 'choice',
default: SORTING_ORDER.DESCENDING,
description: 'Which sorting order to use',
Copy link
Owner

@SanderRonde SanderRonde Feb 24, 2023

Choose a reason for hiding this comment

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

Would put the default in the description here too, so ...to use (default descending). Also would mention this only being available when sorting alphabetically. Also can update the choices to "Sort imports in ascending order alphabetically" and vice versa

Comment on lines +302 to +305
sortingOrder: Util.randomItem([
SORTING_ORDER.ASCENDING,
SORTING_ORDER.DESCENDING,
]),
Copy link
Owner

Choose a reason for hiding this comment

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

I should have done this myself before but can you turn this into Util.randomItem(Object.values(SORTING_ORDER))? Then all cases are covered. Same goes for the line above that I should have done myself ;)

@@ -18,6 +18,7 @@ The plugin will be loaded by Prettier automatically. No configuration needed. It
### Options:

- `sortingMethod`: `'alphabetical' | 'lineLength' (default)` - What to sort the individual lines by. `alphabetical` sorts by the import path and `lineLength` sorts by the length of the import. Note that alphabetical sorting looks at the **whole** import path, so imports starting with `../` are ranked lower.
- `sortingOrder`: `'ascending' | 'descending' (default)` - Determines the order of imports. If set to 'ascending', imports will be sorted in ascending order, whereas if set to 'descending', they will be sorted in descending order
Copy link
Owner

Choose a reason for hiding this comment

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

Would mention here that it's only available when sorting alphabetically. Also the last line is kind of obvious, would either reword that or remove it

@@ -0,0 +1,4388 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

You can delete this file right?

@@ -1,6 +1,6 @@
{
"name": "prettier-plugin-sort-imports",
"version": "1.7.1",
"version": "1.7.2",
Copy link
Owner

Choose a reason for hiding this comment

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

Let's not make this change yet. I'll submit that as a separate patch after yours is merged :)

version "0.8.1"
resolved "https://registry.yarnpkg.com/@cspotcode/source-map-support/-/source-map-support-0.8.1.tgz#00629c35a688e05a88b1cda684fb9d5e73f000a1"
integrity sha512-IchNf6dN4tHoMFIn/7OE8LWZ19Y6q/67Bmf6vnGREv8RSbBVb9LPJxEcnwrcwX6ixSvaiGoomAUvu4YSxXrVgw==
"integrity" "sha512-IchNf6dN4tHoMFIn/7OE8LWZ19Y6q/67Bmf6vnGREv8RSbBVb9LPJxEcnwrcwX6ixSvaiGoomAUvu4YSxXrVgw=="
Copy link
Owner

Choose a reason for hiding this comment

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

Think you can undo the changes to this file right?

@@ -20,6 +25,7 @@ const defaultOptions: Omit<PrettierOptions, keyof ParserOptions> = {
importTypeOrder: [IMPORT_TYPE.ALL],
packageJSONFiles: ['package.json'],
sortingMethod: SORTING_TYPE.LINE_LENGTH,
sortingOrder: SORTING_ORDER.DESCENDING,
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a test case for this? Shouldn't be too hard, just copy one of the alphabetical ones, change the options and invert the order of the expected sorts

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

Successfully merging this pull request may close these issues.

3 participants