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.

Sign up for GitHub

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

Jump to bottom

[FS-672] Improve swagger docs for Client fields. #2657

Merged
merged 8 commits into from
Sep 1, 2022
Merged

[FS-672] Improve swagger docs for Client fields. #2657

merged 8 commits into from
Sep 1, 2022

Conversation

stephen-smith
Copy link
Contributor

FS-672

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • I updated changelog.d subsections with one or more entries.

@stephen-smith stephen-smith temporarily deployed to cachix August 29, 2022 01:19 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 29, 2022 01:19 Inactive
@stephen-smith stephen-smith changed the title Improve swagger docs for Client fields. [FS-672] Improve swagger docs for Client fields. Aug 29, 2022
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 29, 2022
@stephen-smith stephen-smith marked this pull request as ready for review August 29, 2022 03:04
@pcapriotti
Copy link
Contributor

It would be good to also improve the example. Right now it reads:

    "mls_public_keys": {
      "additionalProp1": "string",
      "additionalProp2": "string",
      "additionalProp3": "string"
    },

which is not very useful.

@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 12:48 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 12:48 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 18:25 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 18:25 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 18:42 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 18:42 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 19:48 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 19:48 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 20:00 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix August 31, 2022 20:00 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix September 1, 2022 07:20 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix September 1, 2022 07:20 Inactive
@stephen-smith stephen-smith requested review from fisx and elland September 1, 2022 09:24
fisx
fisx reviewed Sep 1, 2022
View reviewed changes
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

when building this, running http://localhost:8080/swagger-ui/, and opening http://localhost:8080/swagger-ui/, I see this:

2022-09-01-120754_1920x1080_scrot

MLSPublicKeys shouldn't be empty, should it?

libs/schema-profunctor/src/Data/Schema.hs Outdated Show resolved Hide resolved
libs/wire-api/src/Wire/API/User/Client.hs Outdated Show resolved Hide resolved
@fisx fisx temporarily deployed to cachix September 1, 2022 10:12 Inactive
@fisx fisx temporarily deployed to cachix September 1, 2022 10:12 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix September 1, 2022 10:48 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix September 1, 2022 10:48 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix September 1, 2022 10:49 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix September 1, 2022 10:49 Inactive
stephen-smith and others added 8 commits September 1, 2022 05:59
@stephen-smith
docs(swagger): Remove old swagger Client model
7c9ef45 
Old swagger Client model was unused.
@stephen-smith
docs(swagger): Add mls_public_keys description
07477a5
@stephen-smith
docs(swagger): Add base64-specific example string
1a8cbfc
@stephen-smith
docs(swagger): Add MLSPublicKeys example value
f7d3cfc
@stephen-smith
refactor(split): MLSPublicKeys: separate modifiers
6a7e431 
General type modifiers for MLSPublicKeys (like name, description) are
applied in one location, but adapter for use as an optional field named
"mls_public_keys" are done separately.

Also, generalize a HasDescription instance.
@stephen-smith
docs(changelog)
3ad2b93
@stephen-smith @fisx
refactor: where clause taste
e7e6866 
Co-authored-by: fisx <[email protected]>
@stephen-smith @fisx
refactor: allow overlaps of HasExample like HasDescription
461c21a 
Co-authored-by: fisx <[email protected]>
@stephen-smith stephen-smith temporarily deployed to cachix September 1, 2022 10:59 Inactive
@stephen-smith stephen-smith temporarily deployed to cachix September 1, 2022 10:59 Inactive
@stephen-smith
Copy link
Contributor Author

MLSPublicKeys shouldn't be empty, should it?

It doesn't have any fields, so yes, I think it is supposed to be empty in that display.

As best as I can tell, that's normal for map/dict types in swagger.

The example should have the only valid key and an example value, and there should also be a description if you expand the name.

image

image

pcapriotti
pcapriotti approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

LGTM

@stephen-smith stephen-smith merged commit 21805c2 into develop Sep 1, 2022
@stephen-smith stephen-smith deleted the boyd.smith/fs-672 branch September 1, 2022 13:33
@zebot zebot mentioned this pull request Sep 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fisx fisx fisx approved these changes

@pcapriotti pcapriotti pcapriotti approved these changes

@elland elland Awaiting requested review from elland

Assignees
No one assigned
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stephen-smith @pcapriotti @fisx @zebot