Skip to content

Conversation

@blackheaven
Copy link
Contributor

https://wearezeta.atlassian.net/browse/WPB-19257

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@blackheaven blackheaven self-assigned this Aug 13, 2025
@blackheaven blackheaven requested review from a team as code owners August 13, 2025 16:01
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 13, 2025
@blackheaven blackheaven force-pushed the gdifolco/WPB-19257_add-member-count-user-list branch 2 times, most recently from dfbb0c6 to 4beb4b0 Compare August 26, 2025 08:08
@blackheaven blackheaven force-pushed the gdifolco/WPB-19257_add-member-count-user-list branch from b54059c to 2b7241d Compare August 26, 2025 13:11
@blackheaven blackheaven force-pushed the gdifolco/WPB-19257_add-member-count-user-list branch from eee492b to 6dce400 Compare August 26, 2025 16:11
Copy link
Contributor

@battermann battermann left a 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

Comment on lines 910 to 915
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

Copy link
Contributor

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)

Copy link
Contributor Author

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
Copy link
Contributor

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 ...

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks

@blackheaven blackheaven merged commit 2ad4d89 into develop Aug 28, 2025
8 checks passed
@blackheaven blackheaven deleted the gdifolco/WPB-19257_add-member-count-user-list branch August 28, 2025 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants