Don't allow for command name + namespace conflicts about hoard HOT 8 CLOSED

Hey there, I would like to work on this issue but I have some questions.
I can implement command name/namespace checking either here in the with_namespace_input and reissue an input to user again or I can check it here in start function and reissue an input to user before saving it!

But I think there is a disadvantage to latter approach user will finish inputting everything and input name again which some may find off putting!

from hoard.

Hey,
First, thank you for your interest in working on this issue, great to hear!
Your first proposal sounds great to me. As you've suggested, letting the user reissue an input for an alternative command name, if one such command already exists in a given namespace could be the more pleasant solution. Assuming the user more likely than not wants to reissue an alternative name, not a different namespace.

Additionally, you can prompt the user for an alternative namespace , if he wants to keep the command name after all.

from hoard.

I think this is a nice solution, what do you think of this one?

Instead of asking command name first and then namespace, how about asking namespace first, check for name collision and then asking the command name and check for collisions as well?

If this good, I'll make a draft PR soon.

from hoard.

I appreciate your input!
This definitely would make the process much cleaner to implement.
I think we could go with:

• command
• namespace
• name of the command
• description
• tags

While just having the namespace, collision does not yet have to be checked. Only namespace + name shall not collide.

Since a user can import other trove.yml files with new commands, collisions can happen in a different way too (hoard import --file path/to/trove.yml). Thus, your approach should consider this scenario aswell.
One option would be to create a collision check method as part of HoardCommand which checks the namespace + name collision.
It in turn calls with_name_input(String::from(old_name)) which ask for an alternative name. The default value still may show the colliding name, just to remember the user how he wanted to call it in the first place.

Alternatively, dialoguer which provides the dialoging functionality has a .validate_with() (For example used here ). Leveraging this, could decrease complexity. You could make the collision check within that function call. In that way with_name_input() does not have to be called multiple times.

Thoughts on this?

from hoard.

I would like a mix of first and second method, like validating with a method when importing and validating with .validate_with() when using a new command.

What's your input on this?

from hoard.

Sounds great to me. Both can share the same validator method.
Go for it if you feel comfortable with it!

from hoard.

So I started implementing and implemented a validate_name method in HoardCommand (which receives the Trove) but the .validate_with() method accepts a function, that should accept a &String but since I implemented as a method, the methods requires a HoardCommand (and Trove) but I can't create a new object inside the closure because it throws the following error:

error[E0525]: expected a closure that implements the `FnMut` trait, but this closure only implements `FnOnce`
   --> src/command/hoard_command.rs:129:17
    |
128 |             .validate_with({
    |              ------------- the requirement to implement `FnMut` derives from here
129 |                 move |input: &String| -> Result<(), &str> {
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this closure implements `FnOnce`, not `FnMut`
...
132 |                         namespace: self.namespace,
    |                                    -------------- closure is `FnOnce` because it moves the variable `self` out of its environment

Can you give me some insights on this? Should I implement a function that accepts a String for validation?

from hoard.

I'm not sure why you need to construct a new Command object in the closer, can you expand on this?

Thinking about it again, the validator method should be part of Hoard as that struct has knowledge about all stored commands.
When calling .with_name_input() here, you could define a validator method in Hoard and use that as a parameter for .with_name_input which takes a clone of the commands vector in CommandTrove.
I can take a closer look probably tomorrow again.

Alternatively,(a less pretty solution) if that does not work, you could validate the command after running

                let new_command = HoardCommand::default()
                    .with_command_string_input(None)
                    .with_name_input(None)
                    .with_description_input(None)
                    .with_tags_input(None)
                    .with_namespace_input(Some(default_namespace));

Execute this in a loop until there is no naming collision. To keep the user from re-entering the same command, description etc, the defaults can be set as Option<String>

from hoard.