Add write to file option and small improvements #8
Conversation
|
Hi @triole, thank you for the PR. I will fix the CI and check your PR in a few days. Please wait for a while. |
uchiiii
left a comment
uchiiii
left a comment
There was a problem hiding this comment.
Thank you for the pr. I left some comments.
Could you rebase your branch to the latest main?
.gitignore
Outdated
| @@ -1 +1,2 @@ | |||
| .go-version | |||
| build/ | |||
There was a problem hiding this comment.
I added the gitignore rule to exclude my build folder. So we probably do not need this. It was just for my build process. Others might do this differently.
There was a problem hiding this comment.
I see, could you revert this, simply because it is not the same across developers?
There was a problem hiding this comment.
Okay, the line is removed although I do not see how it does any harm.
| &cli.StringFlag{ | ||
| Name: "output", | ||
| Aliases: []string{"o"}, | ||
| Value: "", | ||
| Usage: "Write output to file", | ||
| Destination: &args.OutputFile, | ||
| }, |
There was a problem hiding this comment.
How about csv2md example.csv > test.txt? Is there any use case where you cannot cope with this?
There was a problem hiding this comment.
Yes, there are cases in which I use other tools for CI pipelines for example. Some of them do not support piping because they have their own environments to run shell commands, which sometimes are different from the usual shell in the terminal. I think it does not hurt to have this option and sometimes you will benefit from it.
uchiiii
left a comment
uchiiii
left a comment
There was a problem hiding this comment.
Could you rebase on the latest main and run test?
convert.go
Outdated
| if args.OutputFile == "" { | ||
| fmt.Println(md) | ||
| } else { | ||
| os.WriteFile(args.OutputFile, []byte(md), 0644) |
There was a problem hiding this comment.
os.WriteFile overwrites the content of a file if it exists. Do you think this is user-friendly? Or should we add another option like --force and avoid overwirting by default?
https://pkg.go.dev/os#WriteFile
There was a problem hiding this comment.
Yes, this is a good idea actually. If would also not overwrite existing files. I added this to my fork. Feel free to have a look.
2 participants
Hi there, thanks for this nice tool.
I added a write to file option and made a few small improvements. Feel free to merge this stuff in case you are interested. Otherwise just leave it.
Have a a nice day.