Allow bulk opening multiple files

[koitococo]

Closes #49

[koitococo]

@microsoft-github-policy-service agree

[lhecker]

I'm receiving a lot of comments and PR at the moment, and I may need a few days to get back to this one. I apologize for that.

[diabloproject]

I don't know how "sophisticated" CLI arg parser should be for this project, but the current one does not work like it should. If we are touching it anyway, maybe we can do something like this?

#[derive(Default)]
pub struct Arguments {
    help: bool,
    version: bool,
    files: Vec<OsString>,
}

impl Arguments {
    fn is_option(value: &OsString) -> bool {
        value
            .to_str()
            .map(|x| x.starts_with("-") || (cfg!(windows) && x.starts_with('/')))
            .unwrap_or(false)
    }

    pub fn parse() -> Self {
        let args = env::args_os().collect::<Vec<_>>();
        let options =
            args.clone().into_iter().filter(Self::is_option).map(|x| x.into_string().unwrap());
        let positional = args.into_iter().filter(|x| !Self::is_option(x)).collect::<Vec<_>>();

        let mut this = Self::default();

        for option in options {
            if let Some(name) = option.strip_prefix("--") {
                match name {
                    "help" => this.help = true,
                    "version" => this.version = true,
                    e => {
                        eprintln!("Unkown option {e}");
                        std::process::exit(1);
                    }
                }
            }
            let symbols = if cfg!(windows) && option.starts_with('/') {
                option.strip_prefix('/').unwrap()
            } else {
                option.strip_prefix('-').unwrap()
            };
            for symbol in symbols.chars() {
                match symbol {
                    'h' => this.help = true,
                    'v' => this.version = true,
                    e => {
                        eprintln!("Unkown option {e}");
                        std::process::exit(1);
                    }
                }
            }
        }
        // Right now we do not support any other positional arguments.
        this.files = positional;
        this
    }
}

[lhecker]

I don't mind building a better argument parser (or using a crate for it), and your example code looks great, but I'm not entirely sure I understand in what way it improves the current situation.

[diabloproject]

but I'm not entirely sure I understand in what way it improves the current situation.

If we are talking about this PR, we are just touching this code anyway

In general though, while the exact code can change, we need to handle:

• Proper options: in most CLIs, everything that starts with '-' is an option, and never treated as a file, even if the option does not exist. E.g., when someone tries to write edit -c something.conf, the intention is much more likely to be that they are trying to load a config from a file, even if the option does not exist. We should at least say "hi, I did not understand -c, read help please", and not silently open -c buffer, which both implementations do right now.
• Make parsing code separate from argument handling code: easier to change argument parsing in, e.g. platform-dependent way ('/' on windows is an option, but not on macos, if we want to keep the builds different).

It can be different PR/Issue, it just IMHO more convenient to do in a single one.

[lhecker]

I'd like to do that at a later time and first merge this PR. I believe my suggestion above is not too different from the current code and doesn't really add any new logic per-se. As such I'm fine merging it early. I opened #227 so we can track adding a proper CLI parser. 🙂

[lhecker]

(I'll be blocking some PRs for "Request changes", so I can keep track of where I left notes on and which ones I still need to review.)

[DHowett]

I assume this properly handles e.g. edit /etc/rc.conf /etc/foo /etc/rc.conf by collapsing the two opens of the same document?

[lhecker]

Yes, that should work since the deduplication happens in the DocumentManager.

[lhecker]

I tested your changes and it works great. Thank you for doing this!

I have two more minor requests if you don't mind. If you don't have time right now, let me know and I'll make the changes real quick. 🙂

[lhecker]

cargo clippy complains about this line. Could you change it to this?

Suggested change

	} else if paths.len() == 0 {
	} else if paths.is_empty() {

[lhecker]

Could you separate the modification of cwd and the loop for add_file_path into two pieces? Something like this:

Suggested change

	if paths.len() > 1 {
	for p in &paths {
	state.documents.add_file_path(p)?;
	}
	} else if paths.len() == 1 {
	let p = &paths[0];
	if let Some(parent) = p.parent() {
	cwd = parent.to_path_buf();
	}
	state.documents.add_file_path(p)?;
	}
	for p in &paths {
	state.documents.add_file_path(p)?;
	}
	if let Some(parent) = paths.first().and_then(|p| p.parent()) {
	cwd = parent.to_path_buf();
	}

I think it's shorter and a bit easier to read. It also takes the CWD from the first path which is IMO more consistent behavior.

[koitococo]

I'm sorry that i fotgot to check cargo clippy😞. I will make these changes soon

[lhecker]

Thank you for the PR!