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.

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

Remove non-root swagger definitions #900

Merged

Conversation

cap10morgan
Copy link
Contributor

As per discussion in metosin/reitit#589 (comment) this removes the non-root :definitions from the swagger output.

@cap10morgan
Copy link
Contributor Author

I removed all of the trailing commas in the swagger-test data in 26bda9b, but let me know if you'd rather I not do that. I was having to remove the ones from the deleted non-root definitions in the test expectations and decided it might be better to just clean it all up in one commit first.

@cap10morgan
Copy link
Contributor Author

The one failing test also failed before my changes.

@opqdonut
Copy link
Member

opqdonut commented May 2, 2023

Thanks for removing the commas in a separate commit!

All tests seem to pass now. The failures were caused by a clojars outage, but now all the deps downloaded fine and the tests passed as well.

I have a nagging thought that the :definitions inside :schema might be worth leaving in. I'll investigate a bit and see what various validators say.

@opqdonut
Copy link
Member

opqdonut commented May 2, 2023

Ok, we only need the top-level :definitions since all the references are of the form "$ref": "#/definitions/.... I think this is good to go.

Copy link
Member

@opqdonut opqdonut left a comment

Choose a reason for hiding this comment

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

I'll merge this later today

@opqdonut opqdonut merged commit 1a735fe into metosin:master May 2, 2023
@cap10morgan
Copy link
Contributor Author

cap10morgan commented May 2, 2023

OK, and now with this merged reitit-malli's test expectations need to be updated, at least. I'll work up a PR for that push those changes to the existing PR later today.

@cap10morgan cap10morgan deleted the fix/remove-non-root-swagger-definitions branch May 2, 2023 13:47
@ikitommi ikitommi mentioned this pull request Aug 31, 2023
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.

2 participants