Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

Separate SingleRuleLinter into Linter and LintRule #375

Merged
merged 2 commits into from
Nov 9, 2021
Merged

Separate SingleRuleLinter into Linter and LintRule #375

merged 2 commits into from
Nov 9, 2021

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Nov 6, 2021 •
edited
Loading

Reviewing each commit one by one would be easier, because there are multiple commits, which are too messy when reviewing them together.

@Atry Atry marked this pull request as draft November 6, 2021 04:12
@Atry Atry force-pushed the split-baselinter branch 4 times, most recently from 5bec5e5 to 5f2c53f Compare November 6, 2021 05:35
@Atry Atry changed the title Separate the roles of BaseLinter into Linter and ProblemFinder Separate the roles of BaseLinter into Linter and LintRule Nov 6, 2021
@Atry Atry changed the title Separate the roles of BaseLinter into Linter and LintRule Separate BaseLinter into Linter and LintRule Nov 6, 2021
@Atry Atry marked this pull request as ready for review November 8, 2021 17:45
@Atry Atry requested a review from fredemmott November 8, 2021 17:48
@Atry Atry force-pushed the split-baselinter branch 4 times, most recently from c3577fb to e3d71d6 Compare November 8, 2021 22:20
fredemmott
fredemmott suggested changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

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

The implementation/structure seem great, but the naming is very confusing. Changing it will be a BC break, but I think the clarity/tech-debt avoidance is worth it here.

src/Linters/BaseLinter.hack Outdated Show resolved Hide resolved
src/Linters/LintError.hack Outdated Show resolved Hide resolved
src/Linters/Problem.hack Outdated Show resolved Hide resolved
@Atry Atry marked this pull request as draft November 9, 2021 16:55
@Atry Atry mentioned this pull request Nov 9, 2021
Merged
@Atry Atry marked this pull request as ready for review November 9, 2021 17:07
fredemmott
fredemmott approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

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

Fine to merge as-is, either address before merging or a separate PR

src/Linters/LintError.hack Outdated Show resolved Hide resolved
src/Linters/LintError.hack Show resolved Hide resolved
src/Linters/LintRule.hack Show resolved Hide resolved
src/Linters/Linter.hack Outdated Show resolved Hide resolved
src/Linters/Linter.hack Outdated Show resolved Hide resolved
@Atry Atry force-pushed the split-baselinter branch 2 times, most recently from 251f6dc to 31f3766 Compare November 9, 2021 19:58
@Atry Atry changed the title Separate BaseLinter into Linter and LintRule Separate SingleRuleLinter into Linter and LintRule Nov 9, 2021
@Atry Atry merged commit 281e02c into hhvm:main Nov 9, 2021
@Atry Atry deleted the split-baselinter branch November 9, 2021 20:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fredemmott fredemmott fredemmott approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@Atry @fredemmott @facebook-github-bot