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

feat(specs): add settings spec #17

Merged
merged 3 commits into from
Nov 30, 2021
Merged

feat(specs): add settings spec #17

merged 3 commits into from
Nov 30, 2021

Conversation

shortcuts
Copy link
Member

Summary

  • Add settings spec: getSettings and setSettings methods
  • Add 403 error
  • Move specs/common/responses/common.yml content to specs/common/parameters.yml (used on other clients)
  • Move error files to common (used on other clients)
  • Bumped generator version to 5.3.0 but that did not solve the disclaimer

Disclaimer

There are a lot of duplicate types now, this is because the generator does not create complex types (e.g. with union) when using allOf/oneOf but recreates a complete one instead.

I did multiple combination and was not able to have a cleaner output than this one, but I'll keep trying 🤔

Generating from bundled spec file would solve this since it avoid duplication, but somehow openapi-generator does not resolve paths of bundled specs

@shortcuts
Copy link
Member Author

Tried with the default template for the "duplicate" types, it's the same output so it's either a miss in my spec or a limitation of the generator

Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

LGTM.
I just had a really hard time to figure out how the attributes are ordered in the indexSettings.yml and SearchParams.yml now as the order is completely different from the specs. But I understand the union issues though.
As explained, I think that the name of the object are not self-explanatory enough, that's why I think you should add some comments like:

"params that are only used for index settings"
"params that are only used for search"
"params that are common to index settings and search"

Besides that I think this is really good 🚀

@damcou damcou self-requested a review November 29, 2021 11:22
damcou
damcou previously approved these changes Nov 29, 2021
Copy link
Contributor

@damcou damcou left a comment

Choose a reason for hiding this comment

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

🚀

millotp
millotp previously approved these changes Nov 29, 2021
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

All good for me, minor comments !

specs/common/parameters.yml Show resolved Hide resolved
templates/javascript/model.mustache Outdated Show resolved Hide resolved
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Good for me !

@shortcuts shortcuts merged commit 47f71b9 into main Nov 30, 2021
@shortcuts shortcuts deleted the feat/settings-spec branch November 30, 2021 08:52
shortcuts added a commit that referenced this pull request Apr 22, 2022
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.

3 participants