Add Filename option to hashmap driver to save to local file #26
Conversation
Codecov Report
@@ Coverage Diff @@
## main #26 +/- ##
==========================================
- Coverage 91.16% 89.98% -1.18%
==========================================
Files 7 8 +1
Lines 713 779 +66
==========================================
+ Hits 650 701 +51
- Misses 44 54 +10
- Partials 19 24 +5
|
drivers/hashmap/hashmap.go
Outdated
| content, err = yaml.Marshal(db.data) | ||
| } | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Should we add a bit more context on this error before returning it?
drivers/hashmap/hashmap.go
Outdated
| return err | ||
| } | ||
|
|
||
| return os.WriteFile(db.config.Filename, content, 0755) |
There was a problem hiding this comment.
Same with this, if it returns error a bit more context around it would be useful
There was a problem hiding this comment.
Could we create a table test of valid and invalid file/contents combinations?
drivers/hashmap/byte_slice.go
Outdated
There was a problem hiding this comment.
One thing that might be worth considering is base64 encoding values before writing the json/yaml. Since we accept a byte slice the value can be anything even things that break json or yaml formats.
There was a problem hiding this comment.
Thanks for the comments! I will add more context to the errors that you mentioned and also add some additional tests.
The JSON marshal will automatically encode the byte slice into a base64 string. I think the YAML handles it by making a block scalar. I'll add some more table tests with more complex data to confirm these things. Then I can base64 encode the YAML if there are any issues.
There was a problem hiding this comment.
Ah, good point. I had forgotten that the JSON encoder will automatically base64 to encode a byte slice. But I agree, more tests will help ensure that everything stays the same as packages get updated and more changes are made.
|
I think this is a super cool idea. I like the optional approach where it only happens if the file name is set. Thanks for the pull request it’s super appreciated! |
- Add more context to errors - Test invalid file contents Setup error - Test writing and reading more complex map data to files
Yes, works for me! I don't think I can get the coverage any higher. |
Yeah, I don’t see an easy way to match the same coverage once we start interacting with disk. |
|
Thanks again! |
Description
Add
Filenametohashmap.Configto optionally store data in a YAML or JSON file. This can be useful to run simple applications locally with the option to easily scale to another database backend.I was originally going to add as a separate driver package, but I realized it would mostly be copy/paste of the
hashmapimplementation so I combined it using an optional config value. Let me know if it would be better to move into a separate package.In order to marshal data to YAML, I added the
ByteSlicetype to implement custom Marshal and Unmarshal. The default Marshal would write a[]bytelike an integer array:With the
ByteSlice, it is: