feat(commands)!: add support for flags

[RoloEdits]

Adds the ability to parse out flags for typeable commands.

The implimentation tries to be as straight forward as possible, using no new dependencies, with an emphasis on minimal boilerplate. To this effect, there is a trait introduced to help facilitate extracting and converting to declared types, reducing the setup needed for the good path.

User Experience

Only valid flags are considered, and if any flag-like input is found that doesn't match known flags, it will return with an error printed in the status line.

To deal with edge cases in which the actual input is flag-like, there is a terminator: --. This indicates to the checker that all flags that are expected to be yielded have been. Using this mechanism, you can escape input that is a matching flag name, but you don't wish to be interpreted as a flag:

:write --no-format -- --no-format

:write  -- --no-format

Dev Experience

A basic example of adding flags to a command first involves adding to the flags field:

    TypableCommand {
        name: "write",
        aliases: &["w"],
        signature: CommandSignature {
            flags: &[
                Flag {
                    long: "no-format",
                    short: None,
                    desc: "skips formatting when saving buffer",
                    accepts: None,
                    completer: None,
                }
            ],
            accepts: Some("<path>"),
            positionals: (0, Some(1)),
            parse_mode: ParseMode::Parameters,
            completer: CommandCompleter::positional(&[completers::filename])
         },
        doc: "Write changes to disk. Accepts an optional path (:write some/path.txt)",
        fun: write,
    },

These can be chained for as many flags as needed. long names are passed in with --LONG. If no short is provided, then the only calling convention available would be to use the long name. short can be called with a single dash: -SHORT.

From there, its just a matter of checking for the flags in the callback function body, and checking which ones were found.

Boolean flags can be checked with has_flag:

fn write(cx: &mut compositor::Context, args: Args, event: PromptEvent) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    write_impl(cx, args.first(), false, args.has_flag("no-format"))
}

Flags that themselves have parameters can have their values retrieved with get_flag:

    let register: Option<char> = args.get_flag("register")?;

get_flag can be declared with any primitive type (except bool, that is covered by has_flag) as well as String, &Path, and PathBuf. If the conversion fails, then an error is returned. The str based conversions can never fail.

Long names are used for has_flag and get_flag, with no dash prefix.

Prompt

There is now an accepts: Option<&'static str> field that specifies some extra information for the parsing and the prompt:

This is dynamically added to the prompt, and skipped when None.

Commands

The expectation is that this can be merged with only the few commands changed in this PR, and any other commands can be done in follow-up PRs, using the implementations done here as reference.

Breaking!

• rsort is removed, use :sort --reverse or :sort -r instead
• clear-register no longer clears all registers if no arguments are provided. For the same effect use :clear-register --all or :clear-register -a instead. This change also means that an argument of some kind MUST be provided.

Depends: #12441
Closes: #5828

[RoloEdits]

@the-mikedavis If this looks promising, then I would be willing to see it to merger. If not, then I can wait to get some more feedback before I would think of fully picking this up.

This diff includes changes from the dependent tree. For this pr, there is the flag.rs file, Args::flags in shellwords.rs and then the propagation change in the callback function to take a flags: &[Flag].

The sort command function has the working example implimentation.

[RoloEdits]

One friction point I found was that Args is in helix-core where as the Flag struct is in helix-term. This is the reason for:

    let flags = flags
        .iter()
        .map(|flag| flag.long)
        .chain(flags.iter().filter_map(|flag| flag.short));

as Flag cannot be passed into args.flags directly.

edit: Added a trait IntoFlags in shellwords.rs and than implemented t for Flags. This adds more things to shellwords.rs but the ergonomics should be worth it.

[RoloEdits]

The prompt logic was changed to deal with indicating optional flags and potentially to indicate if a command takes arguments or not.

        let shellwords = Shellwords::from(input);

        if let Some(typed::TypableCommand {
            name,
            aliases,
            flags,
            doc,
            ..
        }) = typed::TYPABLE_COMMAND_MAP.get(shellwords.command())
        {
            // EXAMPLE:
            // write [<flags>] - write the current buffer to its file.
            // aliases: w
            // flags:
            //     --no-format    exclude formatting operation when saving.
            let mut prompt = String::new();

            prompt.push_str(name);

            if !flags.is_empty() {
                prompt.push_str(" [<flags>]");
            }

            writeln!(prompt, " - {doc}").unwrap();

            if !aliases.is_empty() {
                writeln!(prompt, "aliases: {}", aliases.join(", ")).unwrap();
            }

            if !flags.is_empty() {
                prompt.push_str("flags:\n");

                for flag in *flags {
                    write!(prompt, "    --{}", flag.long).unwrap();

                    if let Some(short) = flag.short {
                        write!(prompt, ", -{short}").unwrap();
                    }

                    // TODO: Need to add if the flag takes any arguments?
                    // --all, -a   <arg>    will save all

                    writeln!(prompt, "    {}", flag.desc).unwrap();
                }
            }

            return Some(prompt.into());
        }

        None

Currently it does not show if the command itself takes arguments:

write [<flags>] - write the current buffer to its file.
aliases: w
flags:
    --no-format    exclude formatting operation when saving.

But there could be something like this added:

write [<flags>] ... - write the current buffer to its file.
                ^ ?
aliases: w
flags:
    --no-format ...    exclude formatting operation when saving.

Where then the docs can explain further.

Another option would be to add something like a Takes enum:

enum Takes {
    None,
    One(&str),
    Many(&str),
}

Where there could be something like "[PATH]" or "[PATHS]" added to One and Many respectively, which can be conditionally added like the rest of the prompt parts.

This would entail adding to both the TypeableCommand and Flag struct.

[RoloEdits]

Should also mention up front that no new dependencies where brought in for this. Its unknown if this remains the case going forward.

[RoloEdits]

As brought up here, :write --no-fmt could have an issue of not being able to write a file named --no-fmt, but given how much is exposed to the user when implementing, this could be covered by accounting for if a flag was already encountered:

    let flags = flags
        .iter()
        .map(|flag| flag.long)
        .chain(flags.iter().filter_map(|flag| flag.short));

    let mut encountered = false;

    if let Some(flag) = args.flags(flags) {
        match flag {
            "reverse" | "r" if !encountered => {
                encountered = true;
                sort_impl(cx, true)
            }
            _ => {
                bail!("unhandled command flag `{flag}`, implementation failed to cover all flags.")
            }
        }
    } else {
        sort_impl(cx, false);
    }

The issue of trying to handle it so that all flags can only be given once is that not all commands might want to adhere to this restriction. If Flag was provided, or if the iterator was a (&str, bool) instead, then perhaps something can be built from the knowledge, but as an iterator is lazy, there would be no easy way to skip over already encountered ones and be able to control it externally, as there would need to be bool variables in the calling code anyways.

One other way to handle this was, as said in the comment, to check for --. In this case, the flags method would have this as a check, and if encountered, return None. Added this functionality.

[RoloEdits]

One other limitation is that to keep iterating over the potential flags, the iterator must be cloned, as each iteration all flags must be checked for. This is the main mechanism that allows random order flags. But considering this is dealing with &'static str there shouldn't be any real performance hit.

while let Some(flag) = args.flags(flags.clone()) {}

edit:
Added a trait to shellwords.rs that handles both the iterator boilerplate and the clone handing.

    if let Some(flag) = args.flag(&flags) {
        match flag {
            "reverse" | "r" => sort_impl(cx, true),
            _ => {
                bail!("unhandled command flag `{flag}`, implementation failed to cover all flags.")
            }
        }
    } else {
        sort_impl(cx, false);
    }

[RoloEdits]

Changed

flags: &[flag!{...}]

to

flags: flags![]

And can be provided with flags using

        flags: flags![
            {
                long: "reverse", // required
                short: "r", // optional
                desc: "sort ranges in selection in reverse order" // required
            }
        ],

[the-mikedavis]

The changes look pretty good to me so far. I think we will try to get #11149 merged right after the next release (so it has time to be thoroughly tested in master) and then we can hopefully take a proper look at this within the same release cycle.

The code for flags will probably take some design/discussion. &[Flag] is very straightforward and nicely doesn't need fancy stuff like traits but ideally I'd like the flags to be a little more ergonomic to access and the parsing to less verbose (maybe done within Args somehow). What I'm imagining is something closer to how Clap would expose args or the args.rs in helix-term, so for :sort --reverse for example you might define:

#[derive(Default)]
struct SortFlags {
    reverse: bool,
}

and maybe within the implementation of sort you can ask for that type out of args:

let (flags, args) = args.split_flags::<SortFlags>()?; // returns an Err for unknown flags
let sort_reverse = flags.reverse;

Most typable commands would have a sort of no-op type for this (struct NoFlags;). To do this we would probably need a new trait and a macro to help define it. In contrast to Clap we should really try to avoid a proc macro if possible - I think it should be possible to do something like this with a regular declarative macro. So maybe the actual definition of SortFlags might look like...

command_flags! {
    SortFlags;
    reverse: bool, "r", "sort ranges in selection in reverse order";
}

(or something, that's just rough pseudocode)

And the declarative macro would have enough information to implement a few trait functions that could list the flags, shorthands and descriptions, and Args or whatever could use that information to parse them from the shellwords, show flags in the command_mode popup, etc..

[RoloEdits]

The changes look pretty good to me so far. I think we will try to get #11149 merged right after the next release (so it has time to be thoroughly tested in master) and then we can hopefully take a proper look at this within the same release cycle.

Sounds good to me. I know there are still a few things for that pr that are still unclear for me, as far as expected parsing rules, so testing is a definite must.

As the for the requirements here, I had also thought a typed flags approach was something worth discussing. I agree that the boilerplate currently leaves some to be desired, but wasn't sure how to cover potentially needed bespoke use cases.

For example, here:

let (flags, args) = args.split_flags::<SortFlags>()?; // returns an Err for unknown flags
let sort_reverse = flags.reverse;

In this alone, we have an Args iterator that gets consumed up to the split and then returns a self or &mut self, so args would need to be mut args in the output. The split also seems like it would only ever support an only-flags-first approach. Meaning yank * --join would never work as a valid syntax. And returning on unknown flags means that -- would have to be passed-in in more instances to force a flag-like input to pass this check: :read -- --format.md.

To solve these, perhaps there could be a Args::try_extract_flags<T: Flag>(&self) that would clone &self so that the injected args: Args can be used the same as before. Then we would have:

let flags = args.try_extract_flags::<SortFlags>()?; // Doesn't return error on unknown flags, but tries to populate all known flags.
if flags.reverse { ... }

In this case though, if you want to pass in an argument that has the same name as a flag then you would need to escape it with something not --, as it would go through all arguments to check if there is a match. Though I imagine this collision wont happen often, something would need to be thought up. Another thought point with this approach means that args would need to progress past the flags after the fact, as the this one is not progressed. So a decision would need to be made on where to compromise. Where perhaps it does consume the arg, but only returns the flag struct. This way no need to reassign args. This would restrict the syntax, but might be better for the rest of the experience.

The error returned would then be if the parse function fails if, for example, a flag is expected to itself take an input, and there was none provided. Otherwise, it just returns the default implimentation for the fields. Doing it this way, though, could bring us back to the unknown flags issue.

As for implimentation details, I can imagine something like:

    // impl Args
    fn try_extract_flags<F: Flag>(&self) -> anyhow::Result<F> {
        let mut flag = F::default();

        let mut args = self.clone();

        while let Some(arg) = args.next() {
            flag.extract(arg, &mut args)?;
        }

        Ok(flag)
    }

With the trait something like:

pub trait Flag: Default {
    fn extract<'a, Args: Iterator<Item = &'a str>>(
        &mut self,
        arg: &'a str,
        mut args: &mut Args,
    ) -> anyhow::Result<()> {
        if arg == "--" {
            return Ok(());
        }

        match arg.trim_start_matches("--").trim_start_matches('-') {
                // Would match the field name: join
            "join" => self.NAME = true,
            // Optional short
            "register" | "r" => {
                self.NAME = args
                    .next()
                    // In the Flag implementing struct, could be an `accepts: Option<&'static str>` so that context can
                    // be provided?
                    .context("`NAME` was not not provided a `ACCEPTS`")?
            }
            _ => {}
        }

        Ok(())
    }
}

If the flag takes multiple arguments then would need to handle that as well. Would also decide if -- would ever be considered a valid argument for a flag or if encountered on an expected argument would mean that one wasn't provided.

As for the prompt, this generative change would significantly increase the complexity for its generation, but its probably something that would be best done regardless. For some smaller refactors, making a prompt method on TypeableCommand could clean up some of the other code where its generated now. The callback could then just take command.prompt.

And lastly, for the macros approach as a whole, contributing to changes and general maintenance in this part of the code would become much more complex. I also think there could be some future large changes to the commands themselves to open up specific use cases, with this potentially including generating commands itself, so wonder how much we want to take into account these potential changes now, with this implimentation; keeping a simpler approach or not.

[RoloEdits]

Before I forget, another middle ground approach, if we moved the checks to the try_extract_flags:

    pub fn try_extract_flags<F: Flag>(&mut self) -> anyhow::Result<F> {
        let mut flag = F::default();

        while let Some(arg) = self.next() {
            if arg == "--" {
                return Ok(flag);
            }

            let arg = arg.trim_start_matches("--").trim_start_matches('-');
            
            flag.extract(arg, self)?;
        }

        Ok(flag)
    }

This would only support flag-first-only syntax, but would advance args forward, not needing to do so again after the fact.

With the trait now as:

pub trait Flag: Default {
    fn extract<'a, Args: Iterator<Item = &'a str>>(
        &mut self,
        arg: &'a str,
        args: &mut Args,
    ) -> anyhow::Result<()>;

    fn prompt(&self) -> Option<&'static str> { None }
}

Then when you implement it you would match on the flags like now, but offers a bit more support for bespoke handling.

The prompt part could be implemented here as well, with a convention to follow, but no hard constraints being done for the implementer. It puts more onus on new flag implementors, but id say in one of the most straightforward ways; they can decide if -- is acceptable for their flag now.

In the prompt building phase it would just be a:

if let Some(flags) = command.flags.prompt() { ... }

With the prompt building code moved to TypableCommand::prompt(&self) -> String.

It all comes back to what the dev experience should be like for both implementing, as well as maintaining, the code.

[RoloEdits]

Starting to look into what the implimentation would be like.

To start with, needing to store each type on TypeableCommand in the flags field is going to prove tricky. The Flags trait is not object safe, as Default is a supertrait. But even if we only needed to store it so we can prompt, and make a new trait just for this and have an Box<dyn Prompt>, we run into the issue that the struct needs to be instantiated, as associated functions are also not object safe. And to do this we would need to have a unit type with an implimentation of Prompt. But now we have two different types we are working with.

If the flag! was used to define the structs in a separate file, for organization, then the implementer of the flag would need to pull in a differently named struct than what was defined. I'm not sure about having like:

    TypableCommand {
        name: "read",
        aliases: &["r"],
        flags: flags!{
         // ...
        },
        accepts: Some("<path>"),
        doc: "load a file into buffer",
        fun: read,
        signature: CommandSignature::positional(&[completers::filename]),
    },

I don't know if the struct defined here would be accessible outside of where it was defined in the macro.

We also cant just do a $long: <$type as Default>::default() in a per struct impl, as the try_extract_field function takes Flags to instantiate default values. So it doesn't know about per struct impl's.

I'm sure there are some other things here, but even if we can get them to work, this is still a Box for each command. Don't really like that if at all possible.

On the other side of this, using concrete types with generics seems like a non-starter as we have a slice and a hashmap of TypableCommand, which must all have the same type.

Perhaps I'm just not familiar enough with Rust to know the obvious solution, but it seems to me like without a major refactor around TypableCommand, this cannot be done.

[RoloEdits]

Well, I guess before all that, the const TYPABLE_COMMAND_LIST: &[TypableCommand] isn't able to refer to interior mutability. Would have to make it a static. And even if I were to use a LazyLock or something, i'm pretty sure it would need to be make a new static for each flag to refer to it. Which, even if there is a way to do this in a nice way, would leak memory for all the Box allocations. This is fine in theory, but if there is ever a real memory leak elsewhere, will complicate the process of tracking down.

[RoloEdits]

Pursuing the current implimentation a bit more, as the other way is getting out of hand with "magic", I thought about seeing how I could reduce the boilerplate instead.

Now, along with being able to define the flags right next to the command, flags! handles boilerplate:

    // Flags
    let mut join = false;
    let mut register = String::new();

    flags! {
        for flags, args => {
            "join" | "j" => join = true,
            "register" | "r" => {
                if let Some(reg) = args.next() {
                    register.push_str(reg);
                } else {
                    // Handle if no args were provided to flag
                }
            }
        }
    }

This is something that feels much better than before. The state is still free floating, but its all local still. Building off of what I feel is a strong suit of the current impl, all while being compact:

#[macro_export]
macro_rules! flags {
    // Empty case
    [] => {
        $crate::commands::flag::Flags::empty()
    };
    // Multiple flags case
    [$({ long: $long:expr, $(short: $short:expr,)? desc: $desc:expr $(, accepts: $accepts:expr)? $(,)?}),* $(,)?] => {
        {
            const FLAGS: &[$crate::commands::flag::Flag] = &[
                $(
                    $crate::commands::flag::Flag {
                        long: $long,
                        short: {
                            #[allow(unused_mut, unused_assignments)]
                            let mut short: Option<&'static str> = None;
                            $(short = Some($short);)?
                            short
                        },
                        desc: $desc,
                        accepts: {
                            #[allow(unused_mut, unused_assignments)]
                            let mut accepts: Option<&'static str> = None;
                            $(accepts = Some($accepts);)?
                            accepts
                        },
                    }
                ),*
            ];
            $crate::commands::flag::Flags::new(FLAGS)
        }
    };
    // Extract flags boilerplate
    (for $flags:expr, $args:expr => { $($flag:pat => $action:expr),* $(,)? }) => {
        while let Some(flag) = $args.flag(&$flags) {
            match flag {
                $(
                    $flag => {
                        $action;                    }
                )*
                _ => {
                    anyhow::bail!("unhandled command flag `{}`, implementation failed to cover all flags.", flag);
                }
            }
        }
    };
}

This is in contrast to the monstrosity that was developing with the parsing impl.

[RoloEdits]

Going to open this for review now, as progress is now stopped until a final path forward can be found. Note that there is still a lot of wip work done here and there I have been doing to test/dogfood the changes to see how that are when used. If people are testing this, I would implore to try adding flags to a command to see any rough edges.

I tried to take the path that @the-mikedavis suggested, with a struct containing the state that could be parsed out, but because the typable commands have to be static/const, this lead to issues of needing to have a Lock and Box<dyn> and individual static variables for each flag that would be added. In trying to get the ergonomics for this wrapped around a macro, the macro itself became a monster. I wrote more above. I think there were also issues of the struct that was named cannot be the same one that the user would end up using as it would conflict with the variable name, and I stopped there as it became a little too viral for my liking without more guidance.

I instead went more into the path I started to hopefully keep the implimentation, both the mechanical implimentation as well as the usage implimentation, as simple as possible. While there isn't the nice interface of "deserializing" into a struct, there are only two places that need updating: at the command itself and then the command. Everything is kept as spatially close as possible to the act of refactoring/adding flags. This does have the side effect of some boiler plate, but I added a pattern in the macro to help deal with that as best as I could.

I implemented what I believed to be the command with the most flags, to check the pathological case, and still found this approach pretty straight forward. Dealing with a command that already has flags amortizes most setup as well. Its just adding a pattern and assigning a variable.

In all, although this approach isn't the most ideal possible, I think it works well for its need and isn't out of control complex.

[the-mikedavis]

There's a lot to look at here and I haven't looked at it all but I'd like to see the aliases stuff moved out of the initial flags work. I don't think we need to combine all write/quit/close commands and it seems to be adding a significant amount of complexity and code

[RoloEdits]

I'd like to see the aliases stuff moved out of the initial flags work

I was initially thinking this as well, but without this, it really makes the flags a lot more cumbersome and confusing to use. For example, you have write and write-all and write-force, etc., ones that are plain old writes, but without the aliases, you cant really put --all, --force and get much use out of them as there would be no way to map common extremely common wa usage, as an example. You would still have these other stand alone commands that you would use instead, which crosses wires. And overlap becomes an issue, like with the --no-format, which would need to be added to each write variant. Here, its only once, and can propagate easily for all write actions. No need to duplicate code everywhere.

I don't think we need to combine all write/quit/close commands and it seems to be adding a significant amount of complexity and code

Hmm, I didn't think it was that much code or complexly being added. When refactoring I actually thought it was nice to be able to remove a lot of code. write also seems to be the worst case scenario, which is something I was feeling good about after implementing all the flags, as everything else would not be "worse" than this.

fn write(
    cx: &mut compositor::Context,
    mut args: Args,
    flags: Flags,
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let (_, doc) = current!(cx.editor);

    // Flags
    let mut format = true;
    let mut force = false;
    let mut quit = false;
    let mut all = false;
    let mut close_buffer = false;
    let mut update = false;

    flags! {
        for flags, args => {
            "all" | "a" => all = true,
            "quit" | "q" => quit = true,
            "force" | "f" => force = true,
            "update" | "u" => update = true,
            "no-format" => format = false,
            "close-buffer" => close_buffer = true,
        }
    }

    if all {
        write_all_impl(cx, force, true, format)?;
    } else if update {
        if doc.is_modified() {
            write_impl(cx, args.next(), force, format)?;
        }
    } else {
        write_impl(cx, args.next(), force, format)?;
    }

    if quit && !all {
        cx.block_try_flush_writes()?;
        self::quit(cx, Args::empty(), flags, event)?;
    } else if quit && all {
        quit_all_impl(cx, force)?;
    }

    // After `quit` as that would close helix anyways.
    if close_buffer {
        let doc_ids = buffer_gather_paths_impl(cx.editor, args);
        buffer_close_by_ids_impl(cx, &doc_ids, force)?;
    }

    Ok(())
}

Although this is also something I brought up in custom typeable commands, being able to make an alias for ["write", "quit"] and that we could treat he built-ins as primitives that can be composed, but due to the way the commands are handle not all commands can be composed together deterministically.

[RoloEdits]

@the-mikedavis I have this, in theory, working with a HashMap based flags holder:

fn write(
    cx: &mut compositor::Context,
    mut args: Args,
    flags: Flags,
    event: PromptEvent,
) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    let (_, doc) = current!(cx.editor);

    // Flags
    let all: bool = args.get_flag(["all", "a"])?.unwrap_or_default();
    let quit: bool = args.get_flag(["quit", "q"])?.unwrap_or_default();
    let force: bool = args.get_flag(["force", "f"])?.unwrap_or_default();
    let update: bool = args.get_flag(["update", "u"])?.unwrap_or_default();
    let format: bool = args.get_flag(["no-format"])?.unwrap_or_default();
    let close_buffer: bool = args.get_flag(["close-buffer"])?.unwrap_or_default();

    if all {
        write_all_impl(cx, force, true, format)?;
    } else if update {
        if doc.is_modified() {
            write_impl(cx, args.next(), force, format)?;
        }
    } else {
        write_impl(cx, args.next(), force, format)?;
    }

    if quit && !all {
        cx.block_try_flush_writes()?;
        self::quit(cx, Args::empty(), flags, event)?;
    } else if quit && all {
        quit_all_impl(cx, force)?;
    }

    // After `quit` as that would close helix anyways.
    if close_buffer {
        let doc_ids = buffer_gather_paths_impl(cx.editor, args);
        buffer_close_by_ids_impl(cx, &doc_ids, force)?;
    }

    Ok(())
}

It returns an error if it couldn't convert the value, ?, and then an Option<T> were you can decide what the defaults should be using any flavor of methods. How does this look? is this a clean enough boilerplate?

The rest is the same, declaring with the macro, etc.

The current plan is to take the current Args and rename it to ArgsParser that will have a process of exhausting flags into a HashMap<&str, &str>, and then the rest of the args will be stored in an Vec<Cow<str>>. These will be stored in a new Args.

Sorry to put this here but its all pretty connected.

[the-mikedavis]

An API like that for accessing flags looks pretty good to me but I think we can slim it down even more. As you say we can give the description of the flags to Args when it is parsing so that the parser handles shorthands vs. longhands and we can access flags just by their long names (e.g. "reverse" for -r / --reverse). We will need to understand flags at parsing time (rather than only in the command body) anyways in order to provide completion for flags that take an argument. During parsing then Args would separate positional (regular) args and flags and could detect unknown and duplicated flags. For accessing flags then Args would have an API like:

impl Args {
    // To be preferred for flags that take an argument:
    pub fn get_flag(&self, name: &str) -> Option<&str> { // (maybe a Cow<str>, not sure)
        self.flags.get(name)
    }
    // To be preferred for flags that don't:
    pub fn has_flag(&self, name: &str) -> bool {
        self.flags.contains(name)
    }
}

Kakoune switches are stored as a hashmap of {"<switch long name>": "<switch value>"} even for boolean switches, for example :sort --reverse would be stored in the flags hashmap as {"reverse": ""}. That simplifies some of the value conversions - we wouldn't need the type conversion and extra result.

To be used in a command like so...

fn sort(cx: &mut compositor::Context, args: Args, event: PromptEvent) -> anyhow::Result<()> {
    if event != PromptEvent::Validate {
        return Ok(());
    }

    sort_impl(cx, args.has_flag("reverse"))
}

This is pretty clean so let's discard the idea about a struct+macro+traits for trying to improve ergonomics. I'm not sure that would be possible (and it definitely wouldn't be pretty) without a proc macro anyways.

I would still like to see the aliases stuff moved out. I don't agree that commands like :write and :write-all should be combined and the initial work to add flags should start small and introduce only small examples like the ones mentioned in #5828. For the initial work we could even exclude the code to support flags that take arguments.

[RoloEdits]

That looks good to me, love the has_arg abstraction a lot.

This is pretty clean so let's discard the idea about a struct+macro+traits for trying to improve ergonomics.

I think we still might be able to work with the trait I have for the conversion, so I will keep that for now and then we can decide later on if its work to be able to return declared types vs a &str of some kind and then handling the conversion there.

For the initial work we could even exclude the code to support flags that take arguments.

Id actually like if we can keep this, as its something that will be needed anyways, and the implimentation should be sure it can support it rather than having to find out later its not quite right.

I would still like to see the aliases stuff moved out.

Will do. What about the current macro for declaring the definitions? flags![]. Is that a good approach still? And should I also add a field that takes a completer, like the commands themselves have? Im furthest away on work around the completers so not sure at all about slotting in a new completer for an arg and not a command.

I don't agree that commands like :write and :write-all should be combined

Is there some specific reason that write wouldn't be able to save all buffers? I can understand commands like write-buffer-close or write-quit as they are doing multiple actions, but I think it shouldnt be an issue to combine all and force for write, for example. Though if there is no alias support then we can't do these now anyways.

Im making steady progress with the shellwords refactor. I'm trying to find ways to keep the benefits of the previous implimentation that worked very well for edge cases, and I think its going well. I hope it won't take long to get that ready for review again.

Thanks for all the feedback, sorry its been all over the place.

[the-mikedavis]

I feel that we don't need macros like flag!: &'static [Flag] written inline like we do for TypableCommand is more explicit.

For now the completer field on Flag can be an Option<()>. That can be used to detect whether a flag should accept an argument (or not). Completions for flag args can be done later down the line.

:wa could be written as :write --all for example, it just seems unnecessary to me. Writing all files seems like a separate operation to me than writing the current buffer. And as you say without custom typable commands or aliasing like that, combing those commands here would mean removing :wa

[RoloEdits]

There is still an outstanding use of rest along with the raw_parser. For use with toggle and set. Not sure if you want to take a look and see if there are any clean workarounds for this use case, as I believe if this is solved, then input can be removed from Args, along with the raw_parser method.

(also I will update the main writeup with the changes but will have to be later)
updated

[RoloEdits]

I have already addressed the examples that were given in the issue, but dont know if you want me to add things like :sh --no-popup in here, now, or have separate PRs for all other flags?

[RoloEdits]

As for the docs, should we just leave it the same as now, with no flag information, and let that be discoverable in editor, or should we have something like a column of:

:write:

write [<flags>] <path>: write the current buffer to its file or to <path> if specified

aliases:
    w
flags:
    --no-format        skips formatting when saving buffer

:write!:

write! [<flags>] <path>: write the current buffer to its file or to <path> if specified forcefully

aliases:
    w!
flags:
    --no-format        skips formatting when saving buffer