Skip to content

Migrate data source from rules.json to messages.yaml #9

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

Open
wants to merge 64 commits into
base: main
Choose a base branch
from

Conversation

r0227n
Copy link
Contributor

@r0227n r0227n commented Mar 19, 2025

Issue

Overview (Required)

This pull request includes several changes to the tools/update_lint_rules package, focusing on updating the LintRule model, adding a new JSON converter for State, and modifying the AnalysisOptionsService to use the new State type. The most important changes include the introduction of the State type, the addition of new methods to the RuleState enum, and the updates to the AnalysisOptionsService to filter rules based on the new State type.

Updates to LintRule model:

Enhancements to RuleState enum:

Changes to AnalysisOptionsService:

Codebase simplification:

  • Removed unnecessary fields and methods related to description, incompatibles, sets, fixStatus, and since from the Rule class and its related classes and methods. (tools/update_lint_rules/lib/src/models/lint_rule.freezed.dart, [1] [2] [3] [4] [5] [6] [7] [8]
  • Updated the JSON serialization and deserialization logic in lint_rule.g.dart to reflect the changes in the Rule class. (tools/update_lint_rules/lib/src/models/lint_rule.g.dart, tools/update_lint_rules/lib/src/models/lint_rule.g.dartL17-R50)

Links

Screenshot

N/A

Copy link
Contributor

Ready for review 🚀

Copy link
Member

@blendthink blendthink left a comment

Choose a reason for hiding this comment

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

@r0227n
I've left some comments, so please take a look. 🙏

@r0227n
Copy link
Contributor Author

r0227n commented Mar 21, 2025

@blendthink
Please re-review.
When comparing 3.6/all.yaml before and after the fix, package_api_docs and unsafe_html_attribute are

  • Before: not included
  • After modification: included

but we are aware that this is not a problem.


package_api_docs

Before: rules.json

https://github.com/dart-lang/sdk/blob/ae6da8b926f208bf87d2e11375be5c611c27ee1b/pkg/linter/tool/machine/rules.json#L1625-L1638
The value of state key was not output before the correction because it was removed.

After: message.yaml

https://github.com/dart-lang/sdk/blob/ae6da8b926f208bf87d2e11375be5c611c27ee1b/pkg/linter/messages.yaml#L7394-L7399

Because “removed” is 3.7 or later, it is now output in 3.6.


unsafe_html

Before modification: rules.json

Not output because it is not described in the rules.json.

After: message.yaml

https://github.com/dart-lang/sdk/blob/ae6da8b926f208bf87d2e11375be5c611c27ee1b/pkg/linter/messages.yaml#L13330-L13336

The output is output because it is described.

@r0227n
Copy link
Contributor Author

r0227n commented Apr 2, 2025

@blendthink
Please re-review.🙇

r0227n and others added 19 commits April 10, 2025 12:33
Rename sourceRuleWithCategories to ruleWithCategories to make the code
more concise while maintaining clarity about the variable's role in
handling shared rule categories.
@r0227n
Copy link
Contributor Author

r0227n commented Apr 10, 2025

@blendthink
Please re-review.🙇

Copy link
Member

@blendthink blendthink left a comment

Choose a reason for hiding this comment

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

@r0227n
Apologies for the delayed review comment.
Please take a look and leave your comments or make any necessary changes.

Comment on lines 25 to 36
/// Filter [LintCodeDto] by sharedName
static Iterable<LintCodeDto> filterLintCodeDtosBySharedName(
Iterable<LintCodeDto> dtos,
) {
return dtos.where(
(dto) =>
dto.sharedName == null &&
dto.categories != null &&
dto.deprecatedDetails != null &&
dto.state != null,
);
}
Copy link
Member

Choose a reason for hiding this comment

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

nits-badge
Since this is not a common/shared process, it might be better for readability not to extract it as a separate method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

always_require_non_null_named_parameters has only state and deprecatedDetails, so executing toRule will result in a FormatException.

Therefore, null safety(e.g details ?? initialValue) or add it to the where condition to exclude it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There exist values such as iterable_contains_unrelated_type's where state is not null but categories and deprecatedDetails are null.
Therefore, we changed c8191bc to filter sharedName as null and state as not null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was mistake.
I deleted state in 7d0b4f9 because the conditions sharedName == null and state != null do not exist.

@r0227n
Copy link
Contributor Author

r0227n commented Apr 15, 2025

I confirmed that always_require_non_null_named_parameters is successfully filtered by state by performing the following check because the state has ”removed“: ‘3.3’.

The value of always_require_non_null_named_parameters is

  • exists in build/dart/3.2.yaml
  • Not present in build/dart/3.3.yaml

@r0227n
Copy link
Contributor Author

r0227n commented Apr 15, 2025

@blendthink
Please re-review.🙇

#9 (comment)

Copy link
Member

@blendthink blendthink left a comment

Choose a reason for hiding this comment

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

@r0227n
Please take a look and leave your comments or make any necessary changes.

.map(
(dto) => LintCodeDtoMapper.buildRule(
name: dto.name,
categories: dto.categories ?? const [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since data such as always_require_non_null_named_parameters's for which category is null already exists, an empty character is assigned when category is null.

@r0227n
Copy link
Contributor Author

r0227n commented Apr 16, 2025

@blendthink
Please re-review.🙇

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.

[Improve]: Migrate rules.json to messages.yaml
2 participants