search

hanami / db

The database layer for Hanami
MIT License
7 stars 2 forks source link

Add private interface for transforming a database url #5

Closed alassek closed 5 months ago

alassek commented 5 months ago

This provides a single interface for transforming database urls to be used in hanami/hanami#1395

Since we agree that there is no need to support customization of this logic presently, I believe it would be best to provide a single interface that could be overloaded if someone really needed to change this.

It also makes unit-testing the behavior simpler.

alassek commented 5 months ago

I was unable to push this to a branch in hanami so I had to fork it

alassek commented 5 months ago

@timriley took a stab at a reasonable API location for this functionality, welcome any thoughts you have on this. Unable to formally request review

timriley commented 5 months ago

Thanks @alassek!

Sorry about the issue with repo access — this was a new repo so it didn't have our ordinary team permissions in place. You should be able to write to it now :)

These changes look good to me, only thought is that perhaps we should make a test case like this pass as well?

it "appends _test when there is no detectable environment suffix" do
  expect(subject.call("/bookshelf")).to eq "/bookshelf_test"
end

What do you think? This would replace your "ignores non-conforming paths" case.

I figure with things like this it is better to be safe and err on the side of forcing the separate DB creation?

alassek commented 5 months ago

@timriley good suggestion; I was going to raise the question of how to handle non-conformity in the later implementation but I think this choice would make that unnecessary.

timriley commented 5 months ago

Thanks @alassek! Merge when ready :)