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

Implement adapter Name and add Name constant #65

Merged
merged 7 commits into from
Oct 13, 2023

Conversation

lafriks
Copy link
Contributor

@lafriks lafriks commented Oct 8, 2023

@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4ea1e88) 100.00% compared to head (f36684e) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #65   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          127       130    +3     
=========================================
+ Hits           127       130    +3     
Files Coverage Δ
postgres.go 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lafriks lafriks marked this pull request as draft October 8, 2023 16:40
@lafriks lafriks marked this pull request as ready for review October 10, 2023 15:21
@lafriks
Copy link
Contributor Author

lafriks commented Oct 10, 2023

Ready for review

Coverage can not check panic in MustOpen because db.Open never returns error so not much to be done there :/ At least I could not get it to return error, even for invalid dsn

@lafriks lafriks added the enhancement New feature or request label Oct 10, 2023
@Fs02
Copy link
Member

Fs02 commented Oct 10, 2023

can you do similar to this?
https://github.com/go-rel/rel/blob/master/util.go#L31-L35

so panic can be tested separately

@lafriks
Copy link
Contributor Author

lafriks commented Oct 11, 2023

I don't really see much point in this as this only gives sense of coverage but does not actually test specific case

@lafriks lafriks requested a review from Fs02 October 11, 2023 12:14
@lafriks
Copy link
Contributor Author

lafriks commented Oct 11, 2023

Ok, I think I found a better way to actually make MustOpen to get error and panic so this can be tested now. On a side note it's now possible to use Open and MustOpen functions also with pgx drivers

@lafriks
Copy link
Contributor Author

lafriks commented Oct 12, 2023

@Fs02 gentle ping :)

postgres.go Outdated Show resolved Hide resolved
// Identify if pgx driver is available and default to that instead.
for _, drv := range db.Drivers() {
if drv == "pgx" {
driverName = "pgx"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (maybe not in this PR) I think we should provide a way to override this behavior? such as explicitly setting the driver name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about that but as there are currently no more options I did not do this

@lafriks
Copy link
Contributor Author

lafriks commented Oct 13, 2023

@Fs02 fixed review comment

@lafriks lafriks requested a review from Fs02 October 13, 2023 07:50
Copy link
Member

@Fs02 Fs02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks

@Fs02 Fs02 merged commit db2201d into go-rel:main Oct 13, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants