Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Include Confirmation dialogue if a file is about to be overwritten #86

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Include Confirmation dialogue if a file is about to be overwritten #86

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/beevik/etree"
in "github.com/charmbracelet/freeze/input"
"github.com/charmbracelet/freeze/svg"
"github.com/charmbracelet/huh"
"github.com/charmbracelet/lipgloss"
"github.com/charmbracelet/log"
"github.com/charmbracelet/x/ansi"
Expand Down Expand Up @@ -134,6 +135,45 @@ func main() {
config.Output = defaultOutputFilename
}

istty := isatty.IsTerminal(os.Stdout.Fd())

// Check if file already exists
if _, err := os.Stat(config.Output); err == nil && istty {
var confirm bool
var newOutputFilename string

confirmOverwriteForm :=
huh.NewConfirm().
Title(fmt.Sprintf("'%s' already exists. Would you like to overwrite this file?", config.Output)).
Value(&confirm)
Comment on lines +145 to +148
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should check for if is a directory or not first. If directory, might be better to fail immediately. to save people from themselves.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, I shall get to this in like the next 6 hours or something

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @debdutdeb

image

So I tried the following:

  • freeze.png already exist
  • Ran the following command below
  • Entered the new file name as non-exist/file.png indicating an example of the directory not existing, and adding a file within a non existent directory
    And it failed which is good
go run ./... --execute "cat SECURITY.md"
                                      
   ERROR  Unable to convert SVG to PNG
                                      
  open non-exist/file.png: no such file or directory
                                                    
exit status 1

Obviously when I added it as a parameter for the output, it fails as expected since the file is being added within a non existent directory, which I think is intended

go run ./... --execute "cat SECURITY.md" --output "non-exist/file.png"
                                      
   ERROR  Unable to convert SVG to PNG
                                      
  open non-exist/file.png: no such file or directory
                                                    
exit status 1

And finally, when the form pops up and add the filename as non-exist-dir/non-exist-dir2 to also indicate 2 non existent directory, it fails as expected (yeah even though the non-exist-dir2 would end up becoming a file atleast with me being on Pop OS)

go run ./... --execute "cat SECURITY.md"
                                
   ERROR  Unable to write output
                                
  open non-exist-dir/non-exist-dir2: no such file or directory
                                                              
exit status 1

but yeah should I still go ahead and check if the given path is a directory? To me it seems like it ends up handling it, open to whatever!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does but ig that depends on how file is being written and expection for actions after the confirmation. Like code that writes to the files consider them simply files, and might someday want to remove before writing, which would remove the directory not knowing it is a directory.

So I'd suggest still adding that check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, will look into it within like 24 hours or something!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#86 (comment)

Gone ahead and resolved it here, lemme know if there's anything else!


newOutputFileNameForm := huh.NewInput().
Title("Enter new output filename to use").
Value(&newOutputFilename)

err = confirmOverwriteForm.Run()
if err != nil {
printErrorFatal("could not retrive overwrite confirmation", err)
}

if !confirm {
err := newOutputFileNameForm.Run()
if err != nil {
printErrorFatal("could not retrieve new output filename to use", err)
}

fileInfo, err := os.Stat(newOutputFilename)
if err != nil {
printErrorFatal("could not retrieve new output filename info", err)
}

if fileInfo.IsDir() {
printErrorFatal(fmt.Sprintf("'%s' is a directory. Hence cannot overwrite to the given filename", newOutputFilename), errors.New("could not overwrite to filename"))
}
config.Output = newOutputFilename
}
}

scale = 1
if autoHeight && autoWidth && strings.HasSuffix(config.Output, ".png") {
scale = 4
Expand Down Expand Up @@ -393,8 +433,6 @@ func main() {
}
}

istty := isatty.IsTerminal(os.Stdout.Fd())

switch {
case strings.HasSuffix(config.Output, ".png"):
// use libsvg conversion.
Expand Down