Automatically apply suggestions to source files #158

Merged
merged 5 commits into from Mar 21, 2017

Conversation

Projects
None yet
3 participants
@jpb
Contributor

jpb commented Apr 21, 2016

Introduces --replace and --interactive cli arguments to automatically replace suggestions in source files.

  • The performance isn't great in comparison to regular lein kibit (about a 5x
    slowdown):

    ➜ clojure git:(master)
    ✗ time lein kibit src/clj/clojure/core.clj > /dev/null lein kibit
    src/clj/clojure/core.clj > /dev/null 143.17s user 1.56s system 114% cpu
    2:06.60 total ➜ clojure git:(master) ✗ time lein kibit --replace
    src/clj/clojure/core.clj > /dev/null lein kibit --replace
    src/clj/clojure/core.clj > /dev/null 661.69s user 3.82s system 105% cpu
    10:28.20 total ➜ clojure git:(master) ✗ git diff --stat | cat
    src/clj/clojure/core.clj | 741 ++++++++++++++++++++++-------------------------
    

    Let me know if you notice potential performance issues in this PR
    that I missed, but I think the main slowdown is due to rewrite-clj's parser
    compared to the one kibit.check is using

  • There are some improvements that I want to make to kibit.reporters (#157) which is why the
    "reporters" for replace appear in the kibit.replace namespace

Automatically apply suggestions to source files
Introduces `--replace` and `--interactive` cli arguments to automatically
replace suggestions in source files.

Initial port from jpb/kibit-replace b49410c

Resolves #155
@arrdem

This comment has been minimized.

Show comment
Hide comment
@arrdem

arrdem Apr 21, 2016

Collaborator

Nice! Nothing springs out at me from just reading it, quality time with a profiling tool would probably be in order.

Collaborator

arrdem commented Apr 21, 2016

Nice! Nothing springs out at me from just reading it, quality time with a profiling tool would probably be in order.

jpb added some commits Apr 24, 2016

Don't have check/check-expr traverse the form
replace/check-expr already does a full depth traversal of
the form - previously every form was traversed n times
(n being the depth of the form).
@jpb

This comment has been minimized.

Show comment
Hide comment
@jpb

jpb Apr 24, 2016

Contributor

I discovered that both the replace and check namespaces were doing a full depth traversal of each form - this means that each form was traversed n times, where n is the depth of the form. I fixed this by providing :resolution :subform to check/check-expr.

Times with and without --replace are now about equal:

➜  clojure git:(master) ✗ time lein kibit src/clj/clojure/core.clj > /dev/null
lein kibit src/clj/clojure/core.clj > /dev/null  140.62s user 1.59s system 116% cpu 2:01.88 total
➜  clojure git:(master) ✗ time lein kibit --replace src/clj/clojure/core.clj > /dev/null
lein kibit --replace src/clj/clojure/core.clj > /dev/null  150.87s user 1.81s system 117% cpu 2:09.83 total
➜  clojure git:(master) ✗ git diff --stat | cat
 src/clj/clojure/core.clj | 741 ++++++++++++++++++++++-------------------------
 1 file changed, 349 insertions(+), 392 deletions(-)
Contributor

jpb commented Apr 24, 2016

I discovered that both the replace and check namespaces were doing a full depth traversal of each form - this means that each form was traversed n times, where n is the depth of the form. I fixed this by providing :resolution :subform to check/check-expr.

Times with and without --replace are now about equal:

➜  clojure git:(master) ✗ time lein kibit src/clj/clojure/core.clj > /dev/null
lein kibit src/clj/clojure/core.clj > /dev/null  140.62s user 1.59s system 116% cpu 2:01.88 total
➜  clojure git:(master) ✗ time lein kibit --replace src/clj/clojure/core.clj > /dev/null
lein kibit --replace src/clj/clojure/core.clj > /dev/null  150.87s user 1.81s system 117% cpu 2:09.83 total
➜  clojure git:(master) ✗ git diff --stat | cat
 src/clj/clojure/core.clj | 741 ++++++++++++++++++++++-------------------------
 1 file changed, 349 insertions(+), 392 deletions(-)
@arrdem

This comment has been minimized.

Show comment
Hide comment
@arrdem

arrdem Apr 25, 2016

Collaborator

Now you're cooking with gas 😄

Collaborator

arrdem commented Apr 25, 2016

Now you're cooking with gas 😄

@danielcompton

This comment has been minimized.

Show comment
Hide comment
@danielcompton

danielcompton Apr 25, 2016

Collaborator

Boom. I'll try and get this + the other host of changes into a release this week sometime.

Collaborator

danielcompton commented Apr 25, 2016

Boom. I'll try and get this + the other host of changes into a release this week sometime.

@arrdem

This comment has been minimized.

Show comment
Hide comment
@arrdem

arrdem Oct 11, 2016

Collaborator

Bump bump 😛 This is an awesome feature, anything I can do to help get it merged?

Collaborator

arrdem commented Oct 11, 2016

Bump bump 😛 This is an awesome feature, anything I can do to help get it merged?

@danielcompton

This comment has been minimized.

Show comment
Hide comment
@danielcompton

danielcompton Mar 20, 2017

Collaborator

I don't have time to review this, but if @arrdem is happy then I'm happy.

Collaborator

danielcompton commented Mar 20, 2017

I don't have time to review this, but if @arrdem is happy then I'm happy.

@arrdem

This comment has been minimized.

Show comment
Hide comment
@arrdem

arrdem Mar 21, 2017

Collaborator

I haven't had the time to look at this yet. It also looks like there are some merge conflicts which have come up in the meantime. Unless you have a strong reason to no-ship this at least for a beta grade release I'd be content to merge it to a branch, take a swing at the remaining issues myself and merge it.

Collaborator

arrdem commented Mar 21, 2017

I haven't had the time to look at this yet. It also looks like there are some merge conflicts which have come up in the meantime. Unless you have a strong reason to no-ship this at least for a beta grade release I'd be content to merge it to a branch, take a swing at the remaining issues myself and merge it.

@arrdem arrdem referenced this pull request Mar 21, 2017

Merged

Replace #178

@danielcompton danielcompton merged commit b366ddf into jonase:master Mar 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment