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

Pass ctx to constructor #14

Conversation

ilya-hontarau
Copy link
Contributor

New() makes a few remote calls, so I think it would be great to pass context to this function.
Also in my opinion retrying shouldn't be a part of a constructor(at least if don't specify any configs parameters).

So I made a breaking change(is it okay?), added context to New(), remove retrying, added ErrPing error on failed to ping.

Let me know what do you think.

@craigpastro
Copy link
Owner

Hi @ilya-hontarau! Thank you for your contribution! I think it is a good idea. Tests are failing because there was a breaking change in pgmq recently. I just updated so it should work now if you rebase. I left a few stylistic comments. Thanks again!

@ilya-hontarau ilya-hontarau force-pushed the refactor/pass-context-to-constructor branch from cc439ba to 2ab0c2b Compare September 4, 2023 18:37
pgmq.go Outdated Show resolved Hide resolved
pgmq.go Outdated Show resolved Hide resolved
pgmq_test.go Outdated
"github.com/jackc/pgx/v5/pgconn"
"github.com/stretchr/testify/require"
"github.com/testcontainers/testcontainers-go"
"github.com/testcontainers/testcontainers-go/wait"
"go.uber.org/mock/gomock"

Copy link
Owner

Choose a reason for hiding this comment

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

Nit: remove space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be great to use imports formatters

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I had thought that the goimports linter would have caught this, but it doesn't complain about newlines. Maybe gci works better? Do you know any alternatives?

Copy link
Owner

Choose a reason for hiding this comment

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

It's weird that goimports is not complaining in ci either. Something seems to be not working there. Oh well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@craigpastro actually I like to split imports in 3 group:

  • std
  • external imports
  • internal imports

I tried before GCI, but it still couldnt do this(maybe now they can?).
Also https://github.com/incu6us/goimports-reviser works fine, but it isn't a part of golangci lint:(

Copy link
Owner

Choose a reason for hiding this comment

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

I am pretty happy with

  • std
  • everything else

I am not sure how gci was working in the past but it seems that they can do what you like now by specifying something like:

linters-settings:
  gci:
    sections:
      - standard
      - default
      - prefix(github.com/craigpastro/pgmq-go)

By the way, I think the issue with ci is that the linter config contained:

issues:
  fix: true

If I remove it then it fails when I expect it to. 🤷

@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Patch coverage: 71.42% and project coverage change: -0.61% ⚠️

Comparison is base (4d7cfc2) 80.86% compared to head (a47a61c) 80.25%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
- Coverage   80.86%   80.25%   -0.61%     
==========================================
  Files           1        1              
  Lines         162      157       -5     
==========================================
- Hits          131      126       -5     
- Misses         21       23       +2     
+ Partials       10        8       -2     
Files Changed Coverage Δ
pgmq.go 80.25% <71.42%> (-0.61%) ⬇️

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

Copy link
Owner

@craigpastro craigpastro 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!

@craigpastro craigpastro merged commit 704aee6 into craigpastro:main Sep 4, 2023
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants