active_snapshot icon indicating copy to clipboard operation
active_snapshot copied to clipboard

#15 Using JSON Column instead of YAML encoded object column

Open oskargargas opened this issue 1 year ago • 7 comments

Hello!

I'd love it to be merged into upstream. I've added configuration to switch between YAML and JSON storage types leaving the default with YAML. And on top of that a simple implementation to use PostgreSQL JSONB columns.

Cheers!

oskargargas avatar Aug 19 '22 14:08 oskargargas

Solves #15

westonganger avatar Aug 19 '22 19:08 westonganger

Looks pretty good. I've added some comments. Also can you please add tests for all this code too.

westonganger avatar Aug 19 '22 20:08 westonganger

Looks pretty good. I've added some comments. Also can you please add tests for all this code too.

Nice to hear :) Uploaded fixes to most comments (beside symbol vs string, comment added in conversation). Will add tests when I have a bit more time in work. Hopefully next week.

oskargargas avatar Aug 22 '22 14:08 oskargargas

Hi @oskargargas any chance to work on this yet?

Also can you add a changelog entry in the PR too.

westonganger avatar Aug 29 '22 23:08 westonganger

any chance to work on this yet?

Unfortunately not too much. I've added requested changes but I have literally no idea how to add tests. I would say it is needed to run existing tests but with different migrations and for specific dbs only. And on top of that add some change to CI. I've looked at Rakefile and test_helper how it is setup right now but expanding it is too much for me.

oskargargas avatar Sep 02 '22 11:09 oskargargas

Ok fair, I understand there some larger CI changes that may need to occur.

However can you attempt to add the minimal subset of unit tests? Off the bat I notice the configuration methods can be tested easily.

westonganger avatar Sep 04 '22 19:09 westonganger

Sure. I added tests for config.

oskargargas avatar Sep 05 '22 11:09 oskargargas

Continuing this work over on #32

westonganger avatar Sep 23 '22 09:09 westonganger

Thanks for your work starting this.

Closing this as I have now merged the superseding PR for this to master, #32

westonganger avatar Sep 29 '22 07:09 westonganger