Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add private interface for transforming a database url #5

Merged
merged 1 commit into from
May 28, 2024

Conversation

alassek
Copy link
Contributor

@alassek alassek commented May 25, 2024

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
Copy link
Contributor Author

alassek commented May 25, 2024

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

@alassek
Copy link
Contributor Author

alassek commented May 26, 2024

@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
Copy link
Member

timriley commented May 27, 2024

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
Copy link
Contributor Author

alassek commented May 27, 2024

@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.

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.
@timriley
Copy link
Member

Thanks @alassek! Merge when ready :)

@alassek alassek merged commit eaca280 into hanami:main May 28, 2024
4 checks passed
@alassek alassek deleted the 1395-test-database-url branch May 28, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants