Skip to content

Conversation

@aknuds1
Copy link
Collaborator

@aknuds1 aknuds1 commented Sep 17, 2024

What this PR does:
Add support to the grpcclient package for custom gRPC compressors.

Which issue(s) this PR fixes:

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@aknuds1 aknuds1 added the enhancement New feature or request label Sep 17, 2024
@aknuds1 aknuds1 force-pushed the arve/grpc-comp-custom branch from 54e87f9 to 623d4c9 Compare September 17, 2024 13:17
@aknuds1 aknuds1 requested a review from pstibrany September 17, 2024 13:26
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Looks on the right path.

@aknuds1 aknuds1 requested a review from pstibrany September 17, 2024 13:54
@aknuds1 aknuds1 mentioned this pull request Sep 17, 2024
4 tasks
@aknuds1
Copy link
Collaborator Author

aknuds1 commented Sep 17, 2024

Thanks for the review @pstibrany - I'm going to add tests before finalizing.

Signed-off-by: Arve Knudsen <[email protected]>
@aknuds1 aknuds1 marked this pull request as ready for review September 18, 2024 17:21
@aknuds1
Copy link
Collaborator Author

aknuds1 commented Sep 18, 2024

Added tests.

@aknuds1 aknuds1 merged commit 9102f24 into main Sep 19, 2024
@aknuds1 aknuds1 deleted the arve/grpc-comp-custom branch September 19, 2024 08:02
Comment on lines +11 to +12
func TestConfig(t *testing.T) {
t.Run("custom compressors", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func TestConfig(t *testing.T) {
t.Run("custom compressors", func(t *testing.T) {
func TestCustomCompressorsConfig(t *testing.T) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it's a style issue, but to me it made sense to make the root test for the Config class, and then have sub tests for functionalities.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, not important at all. It just look weird to me to have top-level test function with single t.Run call :) Feel free to ignore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hopefully there will be more sub-tests :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants