-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Compress file content with zstd #46
Conversation
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.
👍 overall looks good, just a few small comments!
thanks again for working on this 💪
Co-authored-by: Rob Herley <robherley@github.com>
Co-authored-by: Rob Herley <robherley@github.com>
Was hoping if you could take a look at the additional setters and getters: When storing the file content to the db we need access to the compressed file content, but I feel like there is a better way to handle this, and I would appreciate it if you could give me any advice! |
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 think just making a public RawContent
field is fine, makes usage in DB pkg a bit easier, left some comments!
Lemme know if you need anything clarified 👍
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.
Looks good! Just a few minor things then we can ship it 🚀
in: []byte("Hello World"), | ||
want: map[string]any{ | ||
"getContent": []byte("Hello World"), | ||
"rawContent": []byte{40, 181, 47, 253, 4, 0, 89, 0, 0, 72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 194, 91, 36, 25}, |
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.
The raw bytes were acquired from the following script:
package main
import (
"fmt"
"github.com/klauspost/compress/zstd"
)
func main() {
encoder, err := zstd.NewWriter(nil)
if err != nil {
panic(err)
}
target := []byte("Hello World")
rawContent := encoder.EncodeAll(target, nil)
for _, val := range rawContent {
fmt.Printf("%d, ", val)
}
}
@robherley Ready for review! |
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.
one last super tiny fix and then we can merge
internal/ssh/handler.go
Outdated
if err != nil { | ||
sesh.Error(err, "Unable to download file", "There was an error downloading the file: %q", file.ID) | ||
} else { | ||
wish.Print(sesh, content) |
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.
wish.Print(sesh, content) | |
wish.Print(sesh, string(content)) |
found this when testing locally 😉 it was printing out the byte slice haha
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! was caught up in the unit tests :(
@robherley
This is a draft pull request that implements file content setter/getter as discussed in #6. I was hoping to get feedback on the core compression and decompression implementations to make sure they align with what you had in mind!
If the implementations are something you had in mind, I'll go ahead and swap
file.Content
statements with getters and setters.