Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Split TrailingCommaRule #1555

Merged
merged 8 commits into from
Aug 7, 2022
Merged

Split TrailingCommaRule #1555

merged 8 commits into from
Aug 7, 2022

Conversation

chao2zhang
Copy link
Contributor

@chao2zhang chao2zhang commented Jul 24, 2022 •
edited
Loading

Fix #1552

Description

Split TrailingCommaRule into OnDeclarationSite and OnCallSite

Checklist

  • PR description added
  • tests are added
  • CHANGELOG.md is updated

In case of adding a new rule:

  • README.md is updated
  • Rule has been applied on Ktlint itself and violations are fixed

chao2zhang
chao2zhang commented Jul 24, 2022
View reviewed changes
.editorconfig Outdated Show resolved Hide resolved
@chao2zhang chao2zhang marked this pull request as ready for review July 24, 2022 15:47
paul-dingemans
paul-dingemans requested changes Jul 24, 2022
View reviewed changes
Copy link
Collaborator

@paul-dingemans paul-dingemans left a comment

Choose a reason for hiding this comment

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

Tnx for the contribution. Please address the review remarks.

.editorconfig Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
...et-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/StandardRuleSetProvider.kt Outdated Show resolved Hide resolved
...tandard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/TrailingCommaOnCallSiteRule.kt Show resolved Hide resolved
.../src/main/kotlin/com/pinterest/ktlint/ruleset/standard/TrailingCommaOnDeclarationSiteRule.kt Show resolved Hide resolved
.../src/main/kotlin/com/pinterest/ktlint/ruleset/standard/TrailingCommaOnDeclarationSiteRule.kt Outdated Show resolved Hide resolved
...tlin/com/pinterest/ktlint/ruleset/standard/internal/trailingcomma/TrailingCommaExtensions.kt Outdated Show resolved Hide resolved
...in/kotlin/com/pinterest/ktlint/ruleset/standard/internal/trailingcomma/TrailingCommaState.kt Outdated Show resolved Hide resolved
@paul-dingemans
Copy link
Collaborator

paul-dingemans commented Jul 24, 2022 •
edited
Loading

Also the documentation pages (not yet formally announced) need to be updated. See file https://github.com/pinterest/ktlint/blob/master/docs/rules/standard.md

New documentation has been released and changed in your branch as well. Can you please update new docs, if not yet done so?

@paul-dingemans
Copy link
Collaborator

I just bumped into test Given an enumeration and the list of values is closed with a semicolon not followed by statements then do not return a lint error. The semi colon after the enum must exist in the AST as in this test the unnecessary semicolon is reported.

@chao2zhang
Copy link
Contributor Author

I just bumped into test Given an enumeration and the list of values is closed with a semicolon not followed by statements then do not return a lint error. The semi colon after the enum must exist in the AST as in this test the unnecessary semicolon is reported.

I am not sure if this is intended for this PR - All the CI checks are passing for me this time with the refactoring as suggested.

@paul-dingemans
Copy link
Collaborator

I just bumped into test Given an enumeration and the list of values is closed with a semicolon not followed by statements then do not return a lint error. The semi colon after the enum must exist in the AST as in this test the unnecessary semicolon is reported.

I am not sure if this is intended for this PR - All the CI checks are passing for me this time with the refactoring as suggested.

Sorry, this remark was meant for the PR regarding the enums and trailing comma's.

@paul-dingemans paul-dingemans mentioned this pull request Aug 2, 2022
Merged
3 tasks
@paul-dingemans
Copy link
Collaborator

@chao2zhang The Trailing comma in enum PR has been merged. Changes are non-trivial, so please be careful with merging changes to your branch.

@chao2zhang chao2zhang force-pushed the split branch 3 times, most recently from 9e227c3 to 35bd5d4 Compare August 5, 2022 04:26
@paul-dingemans
- update changelog and docs
d36597b 
- fix broken visitor-modifier in MaxLineLengthRule
@paul-dingemans paul-dingemans merged commit d64733b into pinterest:master Aug 7, 2022
@chao2zhang chao2zhang mentioned this pull request Sep 19, 2022
Merged
@chao2zhang chao2zhang deleted the split branch October 21, 2022 05:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@paul-dingemans paul-dingemans paul-dingemans requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Supporting flexible configurations of TrailingCommaRule
2 participants
@chao2zhang @paul-dingemans