-
Notifications
You must be signed in to change notification settings - Fork 334
WPB-19257: Add member count to user group list #4714
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
WPB-19257: Add member count to user group list #4714
Conversation
dfbb0c6 to
4beb4b0
Compare
b54059c to
2b7241d
Compare
eee492b to
6dce400
Compare
battermann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve already, but please consider addressing my comment
| instance (ToSchema a) => ToSchema (Identity a) where schema = dimap runIdentity Identity schema | ||
|
|
||
| instance (ToSchema a) => ToSchema (Const a x) where schema = dimap getConst Const schema | ||
|
|
||
| instance (S.ToSchema a, A.ToJSON a, A.FromJSON a) => ToSchema (Maybe a) where schema = genericToSchema | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how that behaves.
If we want to be consistent with the rest of our code base, we can just define the part in the ToSchema instance as follows:
<*> (.membersCount) .= maybe_ (optField "membersCount" schema)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed anymore, dropping it
| <$> (.id_) .= field "id" schema | ||
| <*> (.name) .= field "name" schema | ||
| <*> (.members) .= pure mempty | ||
| <*> (.membersCount) .= field "membersCount" schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be maybe_ (optField ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be good so that the field is omitted when the flag is not provided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, thanks
https://wearezeta.atlassian.net/browse/WPB-19257
Checklist
changelog.d