westonganger / active_snapshot

Simplified snapshots and restoration for ActiveRecord models and associations with a transparent white-box implementation
MIT License
115 stars 18 forks source link

#15 Using JSON Column instead of YAML encoded object column #25

Closed oskargargas closed 2 years ago

oskargargas commented 2 years ago

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!

westonganger commented 2 years ago

Solves #15

westonganger commented 2 years ago

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

oskargargas commented 2 years ago

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.

westonganger commented 2 years ago

Hi @oskargargas any chance to work on this yet?

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

oskargargas commented 2 years ago

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.

westonganger commented 2 years ago

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.

oskargargas commented 2 years ago

Sure. I added tests for config.

westonganger commented 2 years ago

Continuing this work over on #32

westonganger commented 2 years ago

Thanks for your work starting this.

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