jj icon indicating copy to clipboard operation
jj copied to clipboard

cli: allow interactive `squash` and `move` with `paths`

Open crackcomm opened this issue 1 year ago • 3 comments

I think conflicts_with = "interactive" is an artifact and it's now implemented.

crackcomm avatar Nov 18 '23 02:11 crackcomm

Thank you!

It might make sense to add a test. Hopefully, there are already tests that are easy to adapt.

I'm not sure whether this is an absolute necessity, so if it seems troublesome, let me know.

ilyagr avatar Nov 18 '23 04:11 ilyagr

I've been using jj squash -i [paths] and jj move -i [paths] for the past week and it works great.

I'm not sure whether this is an absolute necessity, so if it seems troublesome, let me know.

I think the exact same code path is tested in commit -i but I might be wrong. If anything maybe the tests should be duplicated in case this ever changes. I won't be able to find the time to do it soon though.

crackcomm avatar Nov 27 '23 04:11 crackcomm

I keep postponing looking at this. I was going to double-check that the code path is the same as the one that's tested, but I'm now thinking that wouldn't be sufficient, since this could change in the future.

My main concern with tests is that somebody changes something that breaks jj move -i paths, and will not notice because no tests break. OTOH, they'd have to fix jj commit -i paths, assuming that's tested.

jj currently has a high enough test coverage that I often assume that if I make a change to the code somewhere, the tests will tell me exactly what functionality I affected.

This also doesn't seem urgent. As a result, I'd prefer to wait to merge this until one of us writes these tests (unless the logic is complicated, one test per command should be enough). I'll let you know if I start working on it.

By the way, I do appreciate the PR quite a bit, thank you! It's a nice feature!

Aside: another thing that's missing in this PR is an update to the CHANGELOG. So, I'd say there are the following TODOs:

  • [ ] Tests
  • [ ] Add a changelog entry
  • [ ] Double-check that the docs are consistent with this functionality (i.e., they don't say -i paths is forbidden).

ilyagr avatar Dec 03 '23 23:12 ilyagr