feat: adding delete command and deprecate eject command #157
Conversation
| @@ -278,3 +278,28 @@ func LoadManifest(path string) (*Manifest, error) { | |||
|
|
|||
| return manifest, nil | |||
| } | |||
|
|
|||
| // SaveSauces saves the given sauces to the project directory | |||
| func SaveSauces(projectDir string, sauces []*Sauce) error { | |||
There was a problem hiding this comment.
I think there is no need for this helper function. We should utilize the existing .Save method for Sauce to handle the writing operations. So instead of using this helper function, we should iterate over the sauces slice and use the method:
for _, sauce := range sauces {
err := sauce.Save(opts.Dir)
if err != nil {
...
}
}
| Use: "eject", | ||
| Short: "Remove all Jalapeno-specific files from a project", | ||
| Long: "Remove all the files and directories that are for Jalapeno internal use, and leave only the rendered project files.", | ||
| Deprecated: "use 'delete --all' instead", |
There was a problem hiding this comment.
It's a nice idea to deprecate the command instead of removing it, but since the command is so rarely used we can already remove it completely.
| Long: `Delete sauce(s) from the project. This will remove the rendered files and the sauce entry from sauces.yml. | ||
| If no sauce ID is provided and --all flag is not set, this command will fail.`, | ||
| PreRunE: func(cmd *cobra.Command, args []string) error { | ||
| if len(args) > 0 { |
There was a problem hiding this comment.
Args: cobra.MaximumNArgs(1) should be defined so the user can not provide more than one argument
| Example: `# Delete a specific sauce | ||
| jalapeno delete 21872763-f48e-4728-bc49-57f5898e098a | ||
|
|
||
| # Delete all sauces (same as 'jalapeno eject') |
There was a problem hiding this comment.
If we remove the eject command completely, the note can be removed
| return fmt.Errorf("invalid sauce ID: %w", err) | ||
| } | ||
|
|
||
| sauce, err := recipe.LoadSauceByID(opts.Dir, id) |
There was a problem hiding this comment.
We could refactor the LoadSauceByID method so that it accepts the ID as string. This way we can hide the internal mechanism that the ID is actually UUID. Then LoadSauceByID should validate that is the provided string actually UUID and return an error if it wasn't.
| func NewDeleteCmd() *cobra.Command { | ||
| var opts deleteOptions | ||
| var cmd = &cobra.Command{ | ||
| Use: "delete [SAUCE_ID]", |
There was a problem hiding this comment.
Just a note: I didn't mention this in the issue description, but you've probably also noticed that at the moment there is no clean way to get the sauce ID other than manually looking it up from the sauces.yml file. Later we could have a command to list all the sauces for easier access to IDs.
We should document how to use this command at the end of the usage page (how to get the sauce ID etc).
| if stat, err := os.Stat(jalapenoPath); os.IsNotExist(err) || !stat.IsDir() { | ||
| return fmt.Errorf("'%s' is not a Jalapeno project", opts.Dir) | ||
| } |
There was a problem hiding this comment.
We can check is the directory "a Jalapeno project" by looking the return value of recipe.LoadSauces. If the returned []*Sauce is empty, then we can return an warning "Warning: the directory xxx did not contain any sauces to delete".
There was a problem hiding this comment.
We should also refactor the end-to-end tests to work with this new command.
majori
changed the title
Add delete command and deprecate eject command
Problem
Currently, there's no way to remove specific sauces from a project - the
ejectcommand removes all sauces at once.Solution
Add a new
deletecommand that allows:ejectfunctionality)Changes
deletecommand with two modes:jalapeno delete <sauce-id>: Removes specific sauce and its filesjalapeno delete --all: Removes all sauces and their filesejectcommand as deprecated with message to usedelete --allinsteadTesting
Added comprehensive tests that verify:
Example Usage
Delete specific sauce
Delete all sauces (replaces eject command)