Added Last filter to mimic tail command of shell #19
Conversation
| Thanks for the PR! Don't forget to update the README, and add the new method to the stress-test list, as described in the contributor's guide checklist. |
filters.go Outdated
| return Echo(output.String()) | ||
| } | ||
| | ||
| // Last reads from the pipe, and returns a new pipe containing only the last N |
There was a problem hiding this comment.
Note that the methods in this file are in alphabetical order, per the contributor's guide. It just makes it easier to find stuff.
filters.go Outdated
| return p | ||
| } | ||
| scanner := bufio.NewScanner(p.Reader) | ||
| var stringArr []string |
There was a problem hiding this comment.
In Go we don't tend to name our variables after their type (as someone said, you don't name your pets "Cat" and "Dog"). Sure, this is a slice of strings (not an array), but what's it for? The name should clarify the contents; the compiler can already tell us the type.
filters.go Outdated
| scanner := bufio.NewScanner(p.Reader) | ||
| var stringArr []string | ||
| for scanner.Scan(){ | ||
| stringArr = append(stringArr, scanner.Text()) |
There was a problem hiding this comment.
The problem with this is that we read the whole pipe into memory, before throwing away most of it. Would it be more efficient to use something like a ring buffer that just stores the last X lines?
There was a problem hiding this comment.
I can't think of a more efficient way than to read the whole pipe and the current implementation exposes a Reader, and the Reader interface does not have functions to find the length of the content or going to an arbitrary token index.
That is why to find the last n tokens, we need the number of tokens and for that, we need to read the whole Pipe.
Please let me know if I am wrong or if there is a more efficient method to read the content.
There was a problem hiding this comment.
Well, to take a trivial example, suppose we just want the very last line. We could do something like the following:
- Read a line from the input
- If not the end of input, goto 1.
- Return the line
The question is really how many lines we need to store in memory, given that we want the last X lines. The logical answer is just X, isn't it?
There was a problem hiding this comment.
Yes, will try to incorporate this logic also
filters.go Outdated
| } | ||
| | ||
| if len(stringArr) == 0 { | ||
| return Echo("") |
There was a problem hiding this comment.
If you want an empty pipe, it's simpler to use NewPipe().
filters_test.go Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| input := File("testdata/last.input.txt") |
There was a problem hiding this comment.
Since this file is identical to first.input.txt I suggest we just re-use that file instead.
filters_test.go Outdated
| t.Error("input not closed after reading") | ||
| } | ||
| input = File("testdata/last.input.txt") | ||
| gotZero, err := input.First(0).CountLines() |
There was a problem hiding this comment.
I think this should be Last(0), not First(0).
filters_test.go Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| got, err = File("testdata/last.input.txt").First(100).Bytes() |
| Thanks for the review, will send the updated pr in a day or two |
udithprabhu force-pushed the feature/tail branch from b1b23d6 to bd33203 Compare 
| // Last reads from the pipe, and returns a new pipe containing only the last N | ||
| // lines. If there is an error reading the pipe, the pipe's error status is also | ||
| // set. | ||
| func (p *Pipe) Last(lines int) *Pipe { |
There was a problem hiding this comment.
I have changed the implementation according to your suggestion. Please review
bitfield left a comment
bitfield left a comment There was a problem hiding this comment.
This is looking really good! Excellent examples. Just a few little tweaks and then I think this will be ready to merge.
README.md Outdated
| * [head](examples/head/main.go) | ||
| * [echo](examples/echo/main.go) | ||
| * [tail](examples/tail/main.go) | ||
| * [least frequent](examples/least_freq/main.go) |
There was a problem hiding this comment.
Let's make the name least_freq to match the directory.
| ) | ||
| | ||
| func main() { | ||
| lines, err := strconv.Atoi(os.Args[1]) |
There was a problem hiding this comment.
What happens when there are no command-line arguments?
filters.go Outdated
| input := make([]string, 0, lines) | ||
| for scanner.Scan() { | ||
| if len(input) == lines { | ||
| input = append(input[1:], scanner.Text()) |
There was a problem hiding this comment.
This seems like it might be a little inefficient; have you done any performance testing on it? I wonder if this might be a good place to use the standard library ring package?
filters.go Outdated
| if p == nil || p.Error() != nil { | ||
| return p | ||
| } | ||
| |
There was a problem hiding this comment.
You'll notice that there are no blank lines within any functions or methods in the script package. I tend to think Go code is more readable the more of it you can see at once! For consistency of style, would you mind removing the blank lines?
| Also, don't forget to mention in the PR description, "Fixes #18" (or something like that). Then GitHub will automatically link it and close the issue when the PR is merged. |
| @udithprabhu how's it going? I hope I haven't overwhelmed you with too many changes. If you like, I could pick up this PR and make the requested changes myself, then we can both assess the state of it and see whether it's ready to merge. |
| @bitfield , Sorry for the delay, boggled down by other work, will finish this within 2-3 days if that is fine with you? |
| Not at all! Take all the time you need, your contribution is much appreciated. Just let me know if I can do anything to help. |
udithprabhu force-pushed the feature/tail branch from bd33203 to 6a67201 Compare | @bitfield, I think I am more or less done with all the requested changes. please let me know if further tweaking is necessary. |
| I'll take a look! Thanks very much! |
| Excellent work! Really outstanding. Thanks very much for the contribution. |
2 participants
Added the necessary test function
Added example go program