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.

Sign up for GitHub

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

infra(unicorn): import-style #2901

Merged
merged 4 commits into from
May 31, 2024
Merged

infra(unicorn): import-style #2901

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
974bcd3
infra(unicorn): import-style
ST-DDT May 13, 2024
75f0e42
infra(unicorn): import-style
ST-DDT May 13, 2024
053c527
Merge branch 'next' into infra/unicorn/import-style
ST-DDT May 18, 2024
4848bc2
Merge branch 'next' into infra/unicorn/import-style
ST-DDT May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ module.exports = defineConfig({
'prefer-exponentiation-operator': 'error',
'prefer-template': 'error',

'unicorn/import-style': 'off', // subjective & doesn't do anything for us
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'unicorn/import-style': 'off', // subjective & doesn't do anything for us
'unicorn/import-style': [
'error',
{ styles: { 'node:path': { named: true } } },
],

Copy link
Member Author

@ST-DDT ST-DDT May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tried that - see first commit - and I disagree.
The only thing we are using from that preset is node:path and there we don't use it and the other imports are totally unaffected.
We can talk about that in more detail, once I can talk again, If you would like.

Or let me ask the other way round:

  • What is the benefit of enabling it that way?

Copy link
Member

Choose a reason for hiding this comment

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

For example we use it here:

faker/scripts/apidocs/utils/paths.ts

Line 1 in b27e1a8

import { dirname, resolve } from 'node:path';

with that rule enabled, it prefers import { dirname, resolve } from 'node:path'; over import path from 'node:path';

with that rule set to off, anykind of syntax could be used

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw for that to be the case you have to set default to false.

But what about literally any other import in the entire project?

Copy link
Member

Choose a reason for hiding this comment

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

If you would like, you can find a config for that
I would like to always prefer "named"-imports
I (personally) don't like the "default" of unicorn/import-style

Copy link
Member Author

Choose a reason for hiding this comment

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

If you would like, you can find a config for that

Would you like to give it a try?

I would like to always prefer "named"-imports

Me too, but then there is the locales: https://github.com/faker-js/faker/blob/next/src/locales/cs_CZ/lorem/index.ts#L6

I (personally) don't like the "default" of unicorn/import-style

You can disable them completely with: extendDefaultStyles

Copy link
Member

Choose a reason for hiding this comment

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

I currently have set me a restriction to not actively contribute code and work on code in Open Source this week / until I'm back from USA, so I do a semi-open-source vacation.

Copy link
Member

Choose a reason for hiding this comment

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

@fisker is unicorn/import-style not simply configurable to just and only use named for every import possible? Or is that rule not intended for that usecase?

Copy link

Choose a reason for hiding this comment

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

@fisker is unicorn/import-style not simply configurable to just and only use named for every import possible?

No, not possible.

Or is that rule not intended for that usecase?

You can send a feature request.

Copy link

Choose a reason for hiding this comment

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

What is the benefit of enabling it that way?

It can be useful to avoid different style on the same module in one repo. You can choose any style that works and you like, personally I use named/namespace for node:path too.

'unicorn/no-array-callback-reference': 'off', // reduces readability
'unicorn/no-nested-ternary': 'off', // incompatible with prettier
'unicorn/no-null': 'off', // incompatible with TypeScript
Expand All @@ -50,7 +51,6 @@ module.exports = defineConfig({
// Each rule should be checked whether it should be enabled/configured and the problems fixed, or stay disabled permanently.
'unicorn/better-regex': 'off',
'unicorn/consistent-function-scoping': 'off',
'unicorn/import-style': 'off',
'unicorn/no-object-as-default-parameter': 'off',
'unicorn/numeric-separators-style': 'off',
'unicorn/prefer-export-from': 'off',
Expand Down