When editing in the middle of a rebase, dont clear on quit #873

forivall · 2023-07-20T15:58:05Z

Fixes #739

Also, sets up the fix for #685 by detecting when we're using git-revise, still need to finish that up and conditionally add the git-revise commands of index and cut. I might also want to add special behaviour for index to automatically move the entry to the end (see mystor/git-revise#16 )

Review on the quality of my rust code greatly appreciated!

coveralls · 2023-07-20T17:15:05Z

Pull Request Test Coverage Report for Build 6883077488

45 of 72 (62.5%) changed or added relevant lines in 14 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.4%) to 93.248%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/todo_file/src/errors/io.rs	0	1	0.0%
src/core/src/modules/list/mod.rs	2	4	50.0%
src/core/src/modules/list/utils.rs	6	8	75.0%
src/view/src/testutil/render_view_line.rs	0	2	0.0%
src/todo_file/src/lib.rs	5	8	62.5%
src/todo_file/src/line.rs	1	4	25.0%
src/todo_file/src/search.rs	0	3	0.0%
src/todo_file/src/state.rs	12	15	80.0%
src/display/src/lib.rs	8	12	66.67%
src/todo_file/src/action.rs	2	6	33.33%

Files with Coverage Reduction	New Missed Lines	%
src/git/src/repository.rs	1	87.5%

Totals
Change from base Build 6104199216:	-0.4%
Covered Lines:	4792
Relevant Lines:	5139

💛 - Coveralls

MitMaro

Thanks for this! I did an initial pass, and overall everything looks great!

I left some comments on a few things I noticed. I think the only real thing to solve presently is ensuring that the undo/redo logic is handled, otherwise I believe we can get into an inconsistent state by making a change, and undoing that change.

I also ran the CI pipeline, to see what it would flag. You can run almost everything that the CI pipeline runs by using the cargo make dev command.

MitMaro · 2023-07-20T17:21:58Z

src/todo_file/src/state.rs

+pub enum State {
+	/// Editing todo at start of a rebase.
+	Initial,
+	/// Editing todo in the middle of a rebase with --edit.
+	Edit,
+	/// Editing the todo file for git-revise
+	Revise,
+}


I've had drafts of a similar enum to track the state of the todofile in the past. 👍🏼

It's been half on my personal list of improvements for a bit, but I never had a great reason to add this.

MitMaro · 2023-07-20T17:23:15Z

src/todo_file/src/state.rs

+			return Ok(State::Edit);
+		}
+	}
+	return Ok(State::Initial);


Minor comment here, and I believe Clippy will catch this during linting, but you shouldn't need the return here.

Suggested change

return Ok(State::Initial);

Ok(State::Initial)

MitMaro · 2023-07-20T17:50:14Z

src/todo_file/src/lib.rs

@@ -231,6 +241,7 @@ impl TodoFile {
 			})
 			.collect();
 		self.set_lines(lines?);
+		self.set_state(detect_state(&self.filepath)?);


The more "Rust" way to get the reference here is to use self.filepath.to_path(). They are technically the same thing. In this case, since get_filepath exists, perhaps this:

Suggested change

self.set_state(detect_state(&self.filepath)?);

self.set_state(detect_state(self.get_filepath())?);

Random note: It's generally bad practice to call "getters" get_* in Rust. There existence in this Struct are due to my inexperience when I originally wrote this Struct. This is something that needs to be cleaned up. (Could be a separate PR later, if you are interested.)

Yup, i'm also inexperienced with rust, so that's good to know!

MitMaro · 2023-07-20T17:58:59Z

src/todo_file/src/lib.rs

+	pub const fn state(&self) -> &State {
+		&self.state
+	}


Since State is a cheap "Copy", you can instead just return it directly, without the reference.

Essentially, since the State enum is small in memory, it's just as expensive to create the reference than it is to create a copy of the reference.

Suggested change

pub const fn state(&self) -> &State {

&self.state

}

pub const fn state(&self) -> State {

self.state

}

You will also need to update the usages.

MitMaro · 2023-07-20T17:59:10Z

src/todo_file/src/state.rs

+	Revise,
+}
+
+pub(crate) fn detect_state(filepath: &Path) -> Result<State, IoError> {


Would you mind moving this function in to src/todo_file/src/utils.rs?

MitMaro · 2023-07-20T18:01:03Z

src/todo_file/src/state.rs

+	#[case::edit(State::Initial)]
+	#[case::edit(State::Edit)]
+	#[case::edit(State::Revise)]


Suggested change

#[case::edit(State::Initial)]

#[case::edit(State::Edit)]

#[case::edit(State::Revise)]

#[case::initial(State::Initial)]

#[case::edit(State::Edit)]

#[case::revise(State::Revise)]

MitMaro · 2023-07-20T18:04:30Z

src/todo_file/src/lib.rs

+	/// Set the rebase todo file state.
+	#[inline]
+	pub fn set_state(&mut self, state: State) {
+		self.state = state;


We will want/need to track this in History (self.history), so that undo/redo respects the state change.

The complication here, is that state is a side effect, so it will need to be handled a little differently than the existing history. 🤔

MitMaro · 2023-07-20T18:15:21Z

src/todo_file/src/state.rs

+	if filepath.ends_with("git-revise-todo") {
+		return Ok(State::Revise);
+	}
+	if let Some(parent) = filepath.parent() {


Did you want/mean to use NoParentDir here, if this doesn't exist?

MitMaro · 2023-07-20T18:18:26Z

src/todo_file/src/state.rs

+	use rstest::rstest;
+
+	use super::*;
+	use crate::testutil::{with_todo_revise_file, TodoFileTestDirContext, with_todo_rebase_edit_file, with_todo_rebase_edit_file_stopped};


There are a few formatting issues that can be auto fixed, this line is a good example.

It seems like you are using VSCode, and there is an official plugin for it, that can run format on save.

The other option, is to run cargo make format-apply, and it will fix any formatting issues.

MitMaro · 2023-07-20T18:19:18Z

.vscode/.gitignore

@@ -0,0 +1 @@
+/*


Would you mind adding .vscode to the .gitignore file?

MitMaro · 2023-07-21T12:56:35Z

We will want/need to track this in History (self.history), so that undo/redo respects the state change.

The complication here, is that state is a side effect, so it will need to be handled a little differently than the existing history. thinking

I have an idea here, that can be implemented independently of this PR, and integrated. Let me work on the history, should have something up soonish.

MitMaro · 2023-07-21T13:46:18Z

We will want/need to track this in History (self.history), so that undo/redo respects the state change.
The complication here, is that state is a side effect, so it will need to be handled a little differently than the existing history. thinking

I have an idea here, that can be implemented independently of this PR, and integrated. Let me work on the history, should have something up soonish.

I've put up #877 that should enable handling history.

rpavlik · 2024-11-20T17:17:02Z

Any chance for update and merge of this?

MitMaro reviewed Jul 20, 2023

View reviewed changes

MitMaro assigned MitMaro and forivall Jul 20, 2023

MitMaro mentioned this pull request Jul 21, 2023

Add Operation::Load to History #877

Merged

forivall force-pushed the forivall/edit-and-revise branch from 71cf8ac to ca7f968 Compare July 28, 2023 03:37

forivall force-pushed the forivall/edit-and-revise branch from ca7f968 to 0ab5166 Compare November 15, 2023 21:23

forivall force-pushed the forivall/edit-and-revise branch from 0ab5166 to 8731df9 Compare March 15, 2024 22:52

forivall force-pushed the forivall/edit-and-revise branch from 8731df9 to 648597f Compare June 24, 2024 23:44

forivall added 7 commits July 25, 2024 13:13

chore: vscode settings

74d72bc

Detect rebase state

ff5ef84

Add tests for state detection

6468002

update readme

4953c5f

When editing todo, dont clear

b0f5a48

WIP: add index action for git-revise

f9b62a4

WIP: add cut action for git-revise

e1d1a4e

forivall force-pushed the forivall/edit-and-revise branch from 648597f to e1d1a4e Compare July 25, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When editing in the middle of a rebase, dont clear on quit #873

When editing in the middle of a rebase, dont clear on quit #873

forivall commented Jul 20, 2023

coveralls commented Jul 20, 2023 •

edited

Loading

MitMaro left a comment

MitMaro Jul 20, 2023

MitMaro Jul 20, 2023

MitMaro Jul 20, 2023

forivall Aug 24, 2023

MitMaro Jul 20, 2023

MitMaro Jul 20, 2023

MitMaro Jul 20, 2023

MitMaro Jul 20, 2023

MitMaro Jul 20, 2023

MitMaro Jul 20, 2023

MitMaro Jul 20, 2023

MitMaro commented Jul 21, 2023

MitMaro commented Jul 21, 2023

rpavlik commented Nov 20, 2024

	self.set_state(detect_state(&self.filepath)?);
	self.set_state(detect_state(self.get_filepath())?);

When editing in the middle of a rebase, dont clear on quit #873

Are you sure you want to change the base?

When editing in the middle of a rebase, dont clear on quit #873

Conversation

forivall commented Jul 20, 2023

coveralls commented Jul 20, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6883077488

💛 - Coveralls

MitMaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitMaro commented Jul 21, 2023

MitMaro commented Jul 21, 2023

rpavlik commented Nov 20, 2024

coveralls commented Jul 20, 2023 •

edited

Loading