Skip to content

pool: implemented Role Stringer interface #406

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

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

KaymeKaydex
Copy link

@KaymeKaydex KaymeKaydex commented Sep 4, 2024

implemented Role Stringer interface for human-readable printing

Closes: #405

@KaymeKaydex KaymeKaydex force-pushed the feature/role-stringer branch from 255ca5c to fa6c6f9 Compare September 4, 2024 06:58
@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Sep 4, 2024

I suggest to use stringer (go install golang.org/x/tools/cmd/stringer@latest) + go generate instead of a custom code.

diff --git a/pool/const.go b/pool/const.go
index 7ec239f..22bff4f 100644
--- a/pool/const.go
+++ b/pool/const.go
@@ -30,8 +30,12 @@ const (
 // Role describes a role of an instance by its mode.
 type Role uint32
 
+//go:generate stringer -type Role -linecomment
 const (
-    UnknownRole Role = iota // A connection pool failed to discover mode of the instance.
-    MasterRole              // The instance is read-write mode.
-    ReplicaRole             // The instance is in read-only mode.
+    // A connection pool failed to discover mode of the instance.
+    UnknownRole Role = iota // unknown
+    // The instance is read-write mode.
+    MasterRole              // master
+    // The instance is in read-only mode.
+    ReplicaRole             // replica
 )

@KaymeKaydex KaymeKaydex force-pushed the feature/role-stringer branch 2 times, most recently from 6a062bf to 7135806 Compare September 5, 2024 10:20
@KaymeKaydex
Copy link
Author

@oleg-jukovec i generated Stringer face by stringer library. since it can only cut prefixes, I was forced to rename the roles, since they are not specified externally, this will not change the user interface

@KaymeKaydex KaymeKaydex force-pushed the feature/role-stringer branch 2 times, most recently from 95e6133 to 60e0b2f Compare September 5, 2024 10:32
@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Sep 5, 2024

@oleg-jukovec i generated Stringer face by stringer library. since it can only cut prefixes, I was forced to rename the roles, since they are not specified externally, this will not change the user interface

Unfortunately, we could not change naming because it will break backward compatibility.

But the ‘stringer’ could generate custom strings, see ‘-linecomment’ option for the ‘stringer’ and my patch more carefully.

@KaymeKaydex
Copy link
Author

i will open new pr instead amend fixes

@KaymeKaydex KaymeKaydex reopened this Sep 9, 2024
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Please, fix red tests.

Copy link

@DerekBum DerekBum left a comment

Choose a reason for hiding this comment

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

Thank you for the patch! Please see couple of nits below.

Also, don't forget to squash the commits after the work is done. And please change the line Related issues: #405 to the Closes #405, so the issue could be closed automatically after the merge.

implemented Role Stringer interface for human-readable printing
@KaymeKaydex
Copy link
Author

KaymeKaydex commented Sep 14, 2024

@DerekBum thanks, I accepted the proposed changes and squashed all commits

Copy link

@DerekBum DerekBum left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

@oleg-jukovec oleg-jukovec merged commit 0ccdee2 into tarantool:master Sep 16, 2024
20 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.

Implement Stringer interface for pool.Role
3 participants