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

Automatic replacement of suggestions #155

Closed
jpb opened this issue Apr 4, 2016 · 6 comments
Closed

Automatic replacement of suggestions #155

jpb opened this issue Apr 4, 2016 · 6 comments

Comments

@jpb
Copy link
Contributor

jpb commented Apr 4, 2016

I've created a small plugin that automatically replaces kibit suggestions in source files. Right now it goes something like this:

$ cat your_code.clj
 ...
(+ 1 a)
 ...

$ lein kibit-replace
  Replacing
    (+ 1 a)
   with
    (inc a)
  in your_code.clj:42

$ cat your_code.clj
 ...
(inc a)
 ...

# or 

$ lein kibit-replace --interactive
  Would you like to replace
    (+ 1 a)
   with
    (inc a)
  in your_code.clj:42? [yes/no]

...

Is this something that you would be interested in including in kibit itself?

@danielcompton
Copy link
Member

That's great, I'd be very interested in this. How does it handle preserving whitespace/indentation/line splitting?

@jpb
Copy link
Contributor Author

jpb commented Apr 6, 2016

Great - I'll put together a PR 👍

How does it handle preserving whitespace/indentation/line splitting?

Perserving whitespace, indentation, newlines, and comments is all provided by https://github.com/xsc/rewrite-clj (it looks like it uses a parser combinator, but I haven't dug very much into it https://github.com/xsc/rewrite-clj/blob/master/src/rewrite_clj/parser/core.clj).

@arrdem
Copy link
Collaborator

arrdem commented Apr 8, 2016

So I haven't pulled out a profiler (yet) but just based on running this plugin I'm deeply suspicious of the performance characteristics of this implementation. When linting clojure.core, it takes about 15-30 seconds of I don't know what idling all my cores (so probably thrashing memory) to discover that (first (next x)) in fnext can be replaced with fnext (which is a false positive but shows how long it takes. That's line 108 in Jaunt. Getting the next change on 117 which is (first (first x)) in ffirst's implementation takes another 15-30 seconds. I fully expect to run this overnight on my really beefy box which is mostly sitting idle.

That said I love this idea and definitely want to see something like this in kibit.

@arrdem
Copy link
Collaborator

arrdem commented Apr 8, 2016

Update, started linting Jaunt at 12:56am crashed at 6:22am, did not write a result file. I suspect that something to do with the replacement algorithm is at least quadratic on file size.

@danielcompton
Copy link
Member

Might be a good candidate for http://accidentallyquadratic.tumblr.com once the issue is found.

jpb added a commit to jpb/kibit that referenced this issue Apr 21, 2016
Introduces `--replace` and `--interactive` cli arguments to automatically
replace suggestions in source files.

Initial port from jpb/kibit-replace b49410c

Resolves clj-commons#155
@jpb
Copy link
Contributor Author

jpb commented Apr 21, 2016

@arrdem you are correct in your concern about the performance of kibit-replace - it is definitely quadratic. kibit-replace was mostly a proof of concept - I should advertise it at a such. It re-parses the entire file for each replacement it makes. I've rewritten the algorithm as part of #157 which removes it's quadratic nature.

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

No branches or pull requests

3 participants