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

Jump to bottom

feat(eslint-plugin): renamed exported recommended #2354

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(eslint-plugin): renamed exported recommended #2354

wants to merge 1 commit into from

Conversation

kpanot
Copy link
Contributor

@kpanot kpanot commented Oct 28, 2024

fix(eslint-plugin): turn yaml parser really optional
deprecate(eslint-plugin): recommended exports based on types

Proposed change

feat(eslint-plugin): renamed exported recommended
fix(eslint-plugin): turn yaml parser really optional
deprecate(eslint-plugin): recommended exports based on types

Related issues

@kpanot kpanot requested a review from a team as a code owner October 28, 2024 07:39
matthieu-crouzet
matthieu-crouzet reviewed Oct 28, 2024
View reviewed changes
Copy link
Contributor

@matthieu-crouzet matthieu-crouzet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we do the same ng-update for the @o3r/eslint-config-otter
as the ng-add of @o3r/eslint-plugin does nothing but @o3r/eslint-config-otter does the setup
and in @o3r/eslint-config-otter we should migrate to the not deprecated configs

Moreover do we really need this fix right now as we are working on the ESLint flat config ? or could it be done directly inside the branch for this migration to flat config ?

@kpanot kpanot force-pushed the feature/eslint-plugin-enhance branch from f475e20 to ff81804 Compare October 28, 2024 10:45
@kpanot
Copy link
Contributor Author

kpanot commented Oct 28, 2024 •
edited
Loading

Shouldn't we do the same ng-update for the @o3r/eslint-config-otter as the ng-add of @o3r/eslint-plugin does nothing but @o3r/eslint-config-otter does the setup and in @o3r/eslint-config-otter we should migrate to the not deprecated configs

Indeed, due to technically limitation (fake recursive ref to @o3r/schematics detected by Nx), it is simpler to do it in @o3r/eslint-config-otter.

Moreover do we really need this fix right now as we are working on the ESLint flat config ? or could it be done directly inside the branch for this migration to flat config ?

Nothing requires to have the flat config to fix the exposed recommended.
I think, in contrary, that this should not be done on the already huge linter feature branch

@kpanot kpanot force-pushed the feature/eslint-plugin-enhance branch from ff81804 to b16c2d2 Compare October 28, 2024 14:36
@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Oct 28, 2024 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ca7ae3f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@github-actions github-actions bot added the bug Something isn't working label Oct 28, 2024
@kpanot kpanot closed this Oct 28, 2024
@kpanot kpanot force-pushed the feature/eslint-plugin-enhance branch from b16c2d2 to d4ba8b1 Compare October 28, 2024 14:42
@kpanot kpanot reopened this Oct 28, 2024
@kpanot kpanot force-pushed the feature/eslint-plugin-enhance branch from 453d446 to 0f0c36a Compare October 28, 2024 15:58
@kpanot kpanot force-pushed the feature/eslint-plugin-enhance branch from 0f0c36a to 840dd69 Compare October 29, 2024 08:30
fpaul-1A
fpaul-1A reviewed Oct 29, 2024
View reviewed changes
packages/@o3r/eslint-plugin/src/index.ts
'json-recommended': {
rules: {
'@o3r/json-dependency-versions-harmonize': 'error'
}
},

'angular-template-recommended': {
rules: {
'@o3r/no-inner-html': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

off is the recommended value for this rule?

Copy link
Contributor Author

@kpanot kpanot Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is the current value.
It can be changed, in the big eslint refactor if we want

@kpanot kpanot force-pushed the feature/eslint-plugin-enhance branch from 840dd69 to e8cf18b Compare October 30, 2024 00:48
@kpanot kpanot force-pushed the feature/eslint-plugin-enhance branch from e8cf18b to ca7ae3f Compare October 30, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fpaul-1A fpaul-1A fpaul-1A left review comments

@vscaiceanu-1a vscaiceanu-1a vscaiceanu-1a left review comments

@matthieu-crouzet matthieu-crouzet matthieu-crouzet left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working project:@o3r/eslint-config-otter project:@o3r/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Split the linter plugin to multiple plugins
4 participants
@kpanot @matthieu-crouzet @vscaiceanu-1a @fpaul-1A