search

abingham / traad

An JSON+HTTP server for the rope Python refactoring library
MIT License
107 stars 20 forks source link

Add a "refactoring preview" option for some commands #63

Open abingham opened 10 years ago

abingham commented 10 years ago

It might be cool/useful to be able to see previews for some refactorings so users can decide if they do/don't want to use it.

pigmej commented 10 years ago

:+1: maybe ediff ?

abingham commented 10 years ago

What I had in mind was more like what I see in tools like PyCharm where as you type a new name, you see the results in front of you. The more I think about it, though, the less useful this really seems...it's more like eye-candy, I think. Something like you describe might be the better option, simply letting the user see the changes they'll get.

pigmej commented 10 years ago

I think ediff is quite easy to read and decide "Yeah I want this", in fact it should be possible to select changes that one want to apply (delete ediff content?). Then simple apply patch would work, but that would require changes in refactoring process. Because then whole "reload file" could be also done from emacs (afair apply patch reloads target buffer content, isn't it?)

abingham commented 10 years ago

I don't think it would necessarily require changes to the existing refactoring process. We could design a new "route" from emacs to traad that simply requests the list of changes; traad would not actually apply them. This list could be display to the user in some useful way (e.g. perhaps ediff.) I haven't thought very deeply into this yet, but it seems reasonable.

I'm a bit wary of letting the user select subsets of a refactoring for application. In general, if you apply part of a refactoring you want to apply all of it. Otherwise you run the risk of having an inconsistent code base. There may be some cases where partial application is desirable, but I guess I'd need to see some evidence before I'm convinced..

On Wed, Oct 8, 2014 at 10:59 AM, Jędrzej Nowak notifications@github.com wrote:

I think ediff is quite easy to read and decide "Yeah I want this", in fact it should be possible to select changes that one want to apply (delete ediff content?). Then simple apply patch would work, but that would require changes in refactoring process. Because then whole "reload file" could be also done from emacs (afair apply patch reloads target buffer content, isn't it?)

— Reply to this email directly or view it on GitHub https://github.com/abingham/traad/issues/63#issuecomment-58329010.

pigmej commented 10 years ago

:+1

But

In general, if you apply part of a refactoring you want to apply all of it. Otherwise you run the risk of having an inconsistent code base.

Rope can do something wrong, it can try to refactor some wrong method / fragment. In that case you could ignore it (exclude). Not to exclude refactoring that is done right, but some rope mistakes.

abingham commented 10 years ago

Is this something you see happening a lot? I've never seen it, but it's definitely important to consider if it is happening. The problem I have seen is that rope will miss part of a refactoring, not that makes too many changes.

If you have ideas for how to implement a change that you think will be useful, I'm happy to help you figure out how to make it work. I don't have a lot of time to dedicate to making the change myself, though.

On Wed, Oct 8, 2014 at 1:45 PM, Jędrzej Nowak notifications@github.com wrote:

:+1

But

In general, if you apply part of a refactoring you want to apply all of it. Otherwise you run the risk of having an inconsistent code base.

Rope can do something wrong, it can try to refactor some wrong method / fragment. In that case you could ignore it (exclude). Not to exclude refactoring that is done right, but some rope mistakes.

— Reply to this email directly or view it on GitHub https://github.com/abingham/traad/issues/63#issuecomment-58345469.

pigmej commented 10 years ago

Is this something you see happening a lot? I've never seen it, but it's definitely important to consider if it is happening.

It does happen, not frequently, but if it does, it's kind of "wtf". When it misses some part it's also problematic.

However I'm not rope 'refactoring' fan, except when I really need it :)

If you have ideas for how to implement a change that you think will be useful, I'm happy to help you figure out how to make it work. I don't have a lot of time to dedicate to making the change myself, though.

Sorry, I don't have spare time for it right now to implement it ;/ Maybe some other day ;-)

PythonNut commented 9 years ago

diff-buffer-with-file and C-x C-s if you don't like what you see. revert-buffer otherwise?

abingham commented 9 years ago

@PythonNut I'm worried that an approach like this won't scale beyond single-file refactorings. A lot of important refactorings touch multiple files, and I think it would be a lot to ask users to remember to revert all touched files. Likewise, they'll want to be able to preview refactorings that touch files that don't actually have an open buffer.

pigmej commented 9 years ago

@abingham exactly. Single file refactorings are not that big problem, the real problem is multifiles. Also what with those not yet opened ?

I wish I could trust rope refactorings in 100%, but it sometimes is not happening correctly. So having 'partially' working visual changes would be even worse than none.

pigmej commented 9 years ago

@abingham I'm just wondering if making some "refactoring session id" wouldn't solve problem there.

One would need to start refactoring session by hand (or stated automaticaly), then all refactorings done within this session, would be stored (the original files + changed files) in refactoring sessions dirs (~/.traad/refactorings/{session}) then one could easily inspect what exactly changed, and if needed revert them just by copy file. Also you could then display a buffer with dired in this directory. So diff would be easy also.

let's say M-x traad-begin-session

abingham commented 9 years ago

@pigmej If I understand what you're proposing, then I think rope already does that via the undo/redo feature. Within a given traad session (i.e. a single execution of the traad server), all of the refactorings that have been performed are stored in a stack structure. You can undo the refactorings, popping them over to a redo stack, and by doing this you can selectively undo refactorings.

This doesn't directly give us the ability to review all of the refactorings as, say, diffs. But perhaps that would be simple to add.

There are some good ideas here, but unfortunately I don't have much time to work on traad right now. Hopefully soon...