-
-
Notifications
You must be signed in to change notification settings - Fork 337
added README example that makes replacements in multiple files #230
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.
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
base: master
Are you sure you want to change the base?
Conversation
Nice work @sam-mininberg! |
@@ -220,6 +220,28 @@ script.Echo("a\nb\nc").FilterScan(func(line string, w io.Writer) { | |||
// scanned line: "c" | |||
``` | |||
|
|||
Alternatively, we can use a `Filter` function that returns a `string`. For example, let's fix typos we've made in the CSV files in our working directory: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if referencing CSV files is a red herring here. Readers might think this example is something to do with CSV parsing. Should we just talk about making arbitrary edits to all files matching a pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for disappearing for awhile! Yes I think that's better. I'll make that change.
```go | ||
script.ListFiles("*.csv").FilterLine(func(file string) string { | ||
search := "typ" | ||
replace := "typo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should use completely different search and replace strings, as this particular search and replace would not be idempotent: if you keep running it, "typo" will become "typoo", and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I'll change it to "helo" --> "hello".
script.ListFiles("*.csv").FilterLine(func(file string) string { | ||
search := "typ" | ||
replace := "typo" | ||
s, err := File(file).String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the whole file into a single string is a bit limiting—and unnecessarily so when we only need to read it line by line. What about using script
's own Replace
method, if that's possible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point! I originally chose to read in the whole file like this so that I could use strings.Count
. But I agree with your other comment that the counts are not essential to this example. I'll update my snippet to use Replace
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick update: I'm working on using Replace
but am running into #137. I've yet to come up with an elegant example that works around the issue, but I'll keep at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a temporary file is essential here, isn't it? That's what sed
does (presumably).
replace := "typo" | ||
s, err := File(file).String() | ||
if err != nil { | ||
return fmt.Sprintf("%s %s", file, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better after all to use Filter
, then, since that can return an error and stop the pipe if necessary; something doesn't feel right about just flattening it like this and carrying on. What do you think a sensible text-replacement program should do if it hits an error with some file? What would the equivalent sed
command do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a test script to test what the equivalent sed
would do:
$ bash test.sh
1.txt before sed: helo world!
3.txt before sed: helo there!
Running "sed -i 's/helo/hello/g' 1.txt 2.txt 3.txt"
sed: can't read 2.txt: No such file or directory
1.txt after sed: hello world!
3.txt after sed: hello there!
Even though sed
doesn't behave this way, I do agree with you that it doesn't feel right to continue after encountering an error.
It seems like two approaches are:
- Use
Filter
and stop the pipe at the first error - Use
FilterLine
and put any error messages on the pipe
With all of this in mind, which do you prefer? Or perhaps there is a third approach I'm not considering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you're the customer: as the person who wants to replace text in multiple files, which behaviour would be more convenient or natural to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stopping at the first error seems more natural to me. I'll update my implementation accordingly.
if err != nil { | ||
return fmt.Sprintf("%s %s", file, err) | ||
} | ||
count := strings.Count(s, search) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to have a count of the number of replacements, but not essential—and the key to a good example is eliminating all non-essential detail. I don't deny that we probably would want a real script
text-replacement program to do this, but examples like this are more effective when they're as brief and focused as possible, wouldn't you say?
Addresses #229