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

Update dependencies #1985

Merged
merged 3 commits into from Nov 20, 2022
Merged

Update dependencies #1985

merged 3 commits into from Nov 20, 2022

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented Nov 20, 2022

No description provided.

@fisker fisker marked this pull request as ready for review November 20, 2022 16:34
@sindresorhus
Copy link
Owner

Can you fix the conflict?

# Conflicts:
#	readme.md
@fisker
Copy link
Collaborator Author

fisker commented Nov 20, 2022

I'll be busy with other works from next week, I may won't able to contribute here in near future.

@sindresorhus
Copy link
Owner

I'll be busy with other works from next week, I may won't able to contribute here in near future.

Ok. Thanks for letting me know and no worries :)

@sindresorhus sindresorhus merged commit 349c4fd into sindresorhus:main Nov 20, 2022
@fisker fisker deleted the deps-22-11 branch November 20, 2022 17:27
Comment on lines +49 to +50
💼 [Configurations](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs) enabled in.\
🚫 [Configurations](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs) disabled in.\
Copy link
Contributor

Choose a reason for hiding this comment

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

@bmish The word recommended is missing here

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

That's intentional. The recommended config is mentioned on the next line:

💼 Configurations enabled in.
🚫 Configurations disabled in.
✅ Set in the recommended configuration.

We have separate emojis for each config severity present (error, warn, off) and separate emojis for each config present (in this case, only recommended). This allows us to represent different configurations ("recommended config disables some rule", "foo config sets a rule to warn" etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

But the wording is very off

Copy link
Sponsor Collaborator

@bmish bmish Dec 11, 2022

Choose a reason for hiding this comment

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

Copy link
Contributor

@yakov116 yakov116 Dec 11, 2022

Choose a reason for hiding this comment

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

I think the wording is off, Configurations disabled in. is not a valid English sentence

"in" what?

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

We need to indicate when a rule is explicitly disabled by a config. That's why we have a separate column for that, but it only shows up when there are configs present that explicitly disable rules.

Copy link
Owner

Choose a reason for hiding this comment

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

Why do you need to indicate that? Is there really a difference between not being included in the preset and being disabled in the preset?

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

I think the confusion is happening because eslint-plugin-unicorn sets all non-recommended rules to off in its recommended config, possibly just as a TODO reminder to enable them someday, but doesn't intend to impart any significant meaning from this. Example:

// TODO: Enable this by default when targeting a Node.js version that supports `Array#at`.
'unicorn/prefer-at': 'off',

To solve this in eslint-plugin-unicorn, we could either:

  1. Stop setting rules to off unless specifically necessary.
  2. Or just use --rule-list-columns to hide the column indicating which configs rules are disabled in since this information may not be meaningful in the context of eslint-plugin-unicorn configs.

I do want to note that this setup of explicitly setting all non-recommended rules to "off" is unusual and not something I have seen commonly in other plugins. It's fine if you want to do this, but it can be a bit confusing to automated tooling which has to assume that any configuration is meaningful.

As for other plugins, indications of what configs enable, disable, or warn for rules can all be meaningful, important, and necessary for completeness to fully represent a config's behavior.

This might not seem as important for a simple plugin with just a recommended config, but it becomes apparent when a plugin exports multiple configs or uses various severity levels when configuring its rules.

For example, in the rules list for eslint-plugin-react, react/jsx-uses-react is enabled in the recommended config, but the same rule is then disabled in the jsx-runtime config. I have seen a similar pattern for "prettier" configs which disable rules from a recommended config that conflict with prettier.

So the current information architecture went through several iterations to reach this point where it can accurately represent the configurations in both simple and complex plugins.

Copy link
Owner

Choose a reason for hiding this comment

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

I can appreciate how difficult it is to solve all the needs of different plugins.


Or just use --rule-list-columns to hide the column indicating which configs rules are disabled in since this information may not be meaningful in the context of eslint-plugin-unicorn configs.

👍 I would prefer to just hide the column.


For example, in the rules list for eslint-plugin-react, react/jsx-uses-react is enabled in the recommended config, but the same rule is then disabled in the jsx-runtime config.

I honestly found that one confusing too. I think if we had multiple presets, I would have preferred simply a single emoji indicating each preset and only one column.

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

PR to hide the "configs disabled in" column: #2013

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.

None yet

4 participants