Add a "refactoring preview" option for some commands

[abingham]

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]

👍 maybe ediff ?

[abingham]

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]

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]

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 [email protected]
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
#63 (comment).

[pigmej]

:+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]

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 [email protected]
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
#63 (comment).

[pigmej]

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]

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

[abingham]

@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]

@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]

@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]

@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...