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

Add autocorrect #248

Merged
merged 23 commits into from
Nov 1, 2021
Merged

Add autocorrect #248

merged 23 commits into from
Nov 1, 2021

Conversation

FnControlOption
Copy link
Contributor

@FnControlOption FnControlOption commented Oct 25, 2021

User-facing changes:

  • Added CLI flag --fix.
  • Added autocorrect for Style/LargeNumbers and partially for Layout/TrailingBlankLines.
  • Added optional block parameter to Reportable#add_issue. The block receives a Source::Corrector (described below) to perform autocorrection.
  • Added expect_correction and expect_no_corrections spec helpers.

Implementation details:

  • Added Source::Rewriter and Source::Rewriter::Action, which are ports of Parser::Source::TreeRewriter and Parser::Source::TreeRewriter::Action from the Ruby gem parser. These classes accept source positions as integer indexes (instead of line and column).
  • Added Source::Corrector, which wraps around a Source::Rewriter. This class provides helper methods that accept source locations as Crystal::Location or {Int32, Int32}. (The length of each source line is stored in an array, which is used to convert AST locations to Rewriter positions.)

Changes required for recursive autocorrect (f39a7a4):

  • Added Source#correct, which runs autocorrection and updates the underlying @code property.
  • Issue.new now requires an additional code parameter (because Source#code is no longer immutable).
  • Formatter::Util#affected_code now has an additional method overload.
    • The original method now receives a String as the first parameter instead of Source.
    • The new overload accepts an Issue so that we can now do affected_code(issue) instead of affected_code(issue.code, issue.location, issue.end_location)

Todo:

  • Update other formatters in src/ameba/formatter to indicate whether any issues have been autocorrected.
  • Add missing methods from RuboCop::Cop::Corrector: remove_preceding, remove_leading, remove_trailing
  • Add documentation for:
    • Config#autocorrect
    • Runner#autocorrect
    • Source#correct
    • Source::Corrector and its methods
  • Add :nodoc: directive to Source::Rewriter::Action since it is only used internally.

Future PRs:

src/ameba/runner.cr Outdated Show resolved Hide resolved
src/ameba/cli/cmd.cr Outdated Show resolved Hide resolved
src/ameba/runner.cr Outdated Show resolved Hide resolved
src/ameba/runner.cr Outdated Show resolved Hide resolved
src/ameba/runner.cr Outdated Show resolved Hide resolved
src/ameba/source.cr Outdated Show resolved Hide resolved
src/ameba/source.cr Outdated Show resolved Hide resolved
src/ameba/source.cr Outdated Show resolved Hide resolved
src/ameba/source/rewriter/action.cr Outdated Show resolved Hide resolved
src/ameba/source/rewriter/action.cr Outdated Show resolved Hide resolved
src/ameba/source/rewriter/action.cr Outdated Show resolved Hide resolved
src/ameba/source/rewriter/action.cr Outdated Show resolved Hide resolved
src/ameba/source/rewriter/action.cr Outdated Show resolved Hide resolved
@FnControlOption
Copy link
Contributor Author

Thank you for reviewing, @veelenga and @Sija. Now before I start working on the final todo, do you think it is necessary?

Or is what I have so far good enough, and this can be added in a follow-up pull request?

For reference, here's the relevant documentation: https://github.com/whitequark/parser/blob/master/lib/parser/source/tree_rewriter.rb#L59-L77
(rubydoc.info messes up the formatting)

@veelenga
Copy link
Member

@FnControlOption I am fine moving this out of scope of this PR. Current work is big enough. Thanks

@FnControlOption FnControlOption marked this pull request as ready for review October 28, 2021 07:01
Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

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

I tested this locally and it is looking great. Thanks for you work @FnControlOption

I left two small suggestions/comments

src/ameba/runner.cr Outdated Show resolved Hide resolved
src/ameba/spec/expect_issue.cr Outdated Show resolved Hide resolved
Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

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

👏

Copy link
Member

@Sija Sija left a comment

Choose a reason for hiding this comment

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

🚀

@Sija Sija merged commit 7cb0c15 into crystal-ameba:master Nov 1, 2021
@FnControlOption FnControlOption deleted the autocorrect branch November 16, 2021 23:10
@veelenga veelenga mentioned this pull request Nov 21, 2021
@Sija Sija added the feature label Dec 6, 2021
@Sija Sija added this to the 0.15.0 milestone Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants