Conversation
|
Very cool, thanks for this! What do you think about adding a test? Perhaps you have a toy data file with this type of data in it that we could test against? I am also not a nix user, perhaps @thenonameguy or @craig-latacora or @rschmukler can chime in on that. |
thenonameguy
left a comment
There was a problem hiding this comment.
The nix changes are fine :) I would personally separate out the formatting changes into another PR so there is a cleaner diff on the Clojure side. Tests are automatically run IIRC on nix build, but happy to add that if not.
|
My apologies on the formatting, bad habit of mine not noticing my editor / LSP running the formatter on save. I've removed that from the commit. Agreed with the test too, I'll get one added soon. I don't have an example data file I can share right now for that though, so I'll have to construct one. In my case I'm reading from Iceberg but I don't think that should make a difference. |
|
Don't worry about the formatting at all. Thanks for this contribution. |
|
Looks great! |
|
Sorry for the delay! I have had a full plate for a while, added a specific test for this, it's not very intricate but I think it checks the code path? |
Also updated the clj-nix flake to v4 and regenerated the deps-lock file because I couldn't build without taking these steps. Maybe the better approach would be to not bump clj-nix and instead re-lock with an older version of clj-nix?
I think the issue is that the readme mentions a command to invoke the deps locker too but that is unpinned, so you can generate a newer lockfile than the project build wants to work with. I'm not a Nix user though so I will leave that up to your discretion.
I have not modified any of the write side of things that I think are there because I just don't know enough yet and I only need read side, I assume read is better than nothing. If you would like me to make further changes I'm happy to work with your guidance.
We have published this build at https://clojars.org/st.gower/tmducken for our use internally for now, just in case anyone needs this same change or wants to test this out quickly.
Thanks a lot for the cool tool!