basic file handling #7

emma · 2023-04-12T05:22:53Z

emma commented

2023-04-12 05:22:53 +00:00

No description provided.

emma added 1 commit 2023-04-12 05:22:53 +00:00

aee17365c6 basic file handling

emma added 1 commit 2023-04-12 15:52:29 +00:00

0ff694c6c1 better command parsing with file name argument for :w

emma added 1 commit 2023-04-12 15:55:45 +00:00

c06b39239c Merge remote-tracking branch 'upstream/main'

emma requested review from mars 2023-04-12 15:56:08 +00:00

mars requested changes 2023-04-12 16:27:07 +00:00

mars left a comment

Very, very, very good in general but I had some questions.

src/main.rs Outdated

					
				@ -326,12 +335,60 @@ impl State {

				        }

				    }

				    fn write_buffer(&mut self, file: OsString) -> std::result::Result<(), String> {

mars commented

2023-04-12 16:26:43 +00:00

This is a very good function, but can we make this function return std::io::Result<()> instead? I think that displaying the error as a string should be the job of whatever command is saving the file.

This is a very good function, but can we make this function return `std::io::Result<()>` instead? I think that displaying the error as a string should be the job of whatever command is saving the file.

emma marked this conversation as resolved

src/main.rs

					
				@ -334,0 +361,4 @@

				            None => return Ok(()),

				        };

				        for command in commands.clone().iter() {

mars commented

2023-04-12 16:24:54 +00:00

I don't think that splitting up commands into chars before processing like this is the right approach. For example, q! would be treated as two separate commands despite it performing a different operation than q.

I think if we were going to add commands like q! or wq than those should be matched on the corresponding branches. Making write_buffer() as a separate function definitely makes doing that easier, so that was the right call.

I don't think that splitting up commands into chars before processing like this is the right approach. For example, `q!` would be treated as two separate commands despite it performing a different operation than `q`. I think if we were going to add commands like `q!` or `wq` than those should be matched on the corresponding branches. Making `write_buffer()` as a separate function definitely makes doing that easier, so that was the right call.

emma commented

2023-04-12 16:47:27 +00:00

I was planning on making the Iterator peekable, so we could do:

'q' => {
	if commands.peek() == '!' { self.no_save = true; }
	else { self.no_save = false; }
    self.quit = true;
},

I was planning on making the Iterator peekable, so we could do: ```rs 'q' => { if commands.peek() == '!' { self.no_save = true; } else { self.no_save = false; } self.quit = true; }, ```

emma commented

2023-04-12 16:48:11 +00:00

or more likely matching the !, but idk if that would be good or not

or more likely `match`ing the `!`, but idk if that would be good or not

mars commented

2023-04-12 16:51:48 +00:00

I think it's much more straight-forward just to match the strings.

emma commented

2023-04-12 17:01:16 +00:00

Probably. I’ll try that out

src/main.rs

					
				@ -334,0 +363,4 @@

				        for command in commands.clone().iter() {

				            match command {

				                'q' => self.quit = true,

mars commented

2023-04-12 16:21:13 +00:00

If "q" is given multiple command parts, it still quits without issuing a warning. I'm not sure if this should exit silently even when given more arguments than expected. What do you think?

Edit: how we decide to do this now will help us handle argc differences in the future.

If "q" is given multiple command parts, it still quits without issuing a warning. I'm not sure if this should exit silently even when given more arguments than expected. What do you think? Edit: how we decide to do this now will help us handle argc differences in the future.

emma commented

2023-04-12 16:49:18 +00:00

It definitely should not exit silently, it should error, stating that :q doesn’t take arguments (on its own).

It definitely should not exit silently, it should error, stating that `:q` doesn’t take arguments (on its own).

src/main.rs

					
				@ -334,0 +370,4 @@

				                    if let Some(part) = self.file.clone() {

				                        handle = part;

				                    } else {

				                        handle = match command_parts.get(1) {