avito-tech / go-transaction-manager

Transaction manager for GoLang
MIT License
222 stars 12 forks source link

feat: Implement pgx support #76

Closed hound672 closed 1 year ago

hound672 commented 1 year ago

Hello. The frist attempt to implement pgx support. For now there is no any unittests, example is weird and so on. I just wonder if I am on the right way.

hound672 commented 1 year ago

remove main.go, docker-compose. write unittests for transaction and for settings.


sqlmock cannot be used here due to different interfaces so I used pgxmock lib

maranqz commented 1 year ago

I'll take a closer look at the weekend.

hound672 commented 1 year ago

Looks like pgxmock does not work on go1.13. I had the same issues with go1.13.

maranqz commented 1 year ago

If it's true, mark it in README.md and add skip test for old version of Go. GORM adapterfor example.

hound672 commented 1 year ago

If it's true, mark it in README.md and add skip test for old version of Go. GORM adapterfor example.

Ok, I'll try. So this an another reason why I think that separated repositories is a good option.

maranqz commented 1 year ago
  1. I see in pgx repo This is the previous stable v4 release. v5 been released. Is v4 still actual? https://github.com/jackc/pgx/tree/v4.18.1

  2. Let's call the adapter pgx v4 to show the version explicitly.

hound672 commented 1 year ago
  1. I see in pgx repo This is the previous stable v4 release. v5 been released. Is v4 still actual? https://github.com/jackc/pgx/tree/v4.18.1
  2. Let's call the adapter pgx v4 to show the version explicitly.

In the package's name? Like pgx -> pgx_v4?

hound672 commented 1 year ago
  1. I see in pgx repo This is the previous stable v4 release. v5 been released. Is v4 still actual? https://github.com/jackc/pgx/tree/v4.18.1
  2. Let's call the adapter pgx v4 to show the version explicitly.

added v5 as well

hound672 commented 1 year ago

Linter: I checked with 1.49 linter version and there are no any issues. And I will not modify files in trmanager.

maranqz commented 1 year ago

Linter: I checked with 1.49 linter version and there are no any issues. And I will not modify files in trmanager.

I'm fixing it)

hound672 commented 1 year ago

Linter: I checked with 1.49 linter version and there are no any issues. And I will not modify files in trmanager.

I'm fixing it)

It's pretty strange) what's wrong?)

maranqz commented 1 year ago

golangci-lint version is 1.56, I think something changed in it.

I excluded mock dirs.

hound672 commented 1 year ago

golangci-lint version is 1.56, I think something changed in it.

I excluded mock dirs.

I see, excluding. But why does it work before these changes?

maranqz commented 1 year ago

I think, cause is updating of exhaustruct: from 2.3.0 to 3.1.0 https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md#v1540

hound672 commented 1 year ago

Pgx v5 works only with 1.19 and higher (due to generics). But in example should be two flags:

  1. Works only with real db
  2. Build only with 1.19 and higher.
maranqz commented 1 year ago

Build only with 1.19 and higher.

Use for pgxv5

//go:build go1.19,with_real_db
// +build go1.19,with_real_db
Space-separated elements // +build pro enterprise pro OR enterprise
Comma-separated elements // +build pro,enterprise pro AND enterprise
Exclamation point elements // +build !pro NOT pro
hound672 commented 1 year ago

done. Actions passed after changes (in my env).

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 5915867691


Changes Missing Coverage Covered Lines Changed/Added Lines %
pgxv4/settings.go 39 42 92.86%
pgxv5/settings.go 40 43 93.02%
pgxv4/context.go 12 18 66.67%
pgxv5/context.go 12 18 66.67%
<!-- Total: 221 239 92.47% -->
Totals Coverage Status
Change from base Build 5857614173: -0.3%
Covered Lines: 1461
Relevant Lines: 1554

💛 - Coveralls
maranqz commented 1 year ago

Do you want to fix something else in the PR? I think PR is ready to merge. Good job and thank you for your work👍🎉🚀.

hound672 commented 1 year ago

Do you want to fix something else in the PR? I think PR is ready to merge. Good job and thank you for your work👍🎉🚀.

Thanks!) Could you merge? Since I'm not authorized for that

maranqz commented 1 year ago

I will bump version of the lib in first September weekend.