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

Fixed usage of correct types in enums #1117

Merged
merged 4 commits into from
Nov 1, 2021

Conversation

FlorianRappl
Copy link
Contributor

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests? (adjusted ex. tests)
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)? (adjusted ex. tests)
  • This PR is associated with an existing issue?

Closing issues

Put closes #1068 in your comment to auto-close the issue that your PR fixes.

If this is a new feature submission:

(in some sense it yes, so I'll fill this one out, too)

  • Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach

Only targets OAS 3, but this seems to be the target of the issue (and is the current / default spec). It might be "breaking", but then again, it's only the OAS output and not the validator. Since this one even follows OAS more closely than beforehand I doubt it should have any negative impact though. Tooling etc. would like the "new style" even more than the old one.

Test plan

Some existing tests only build upon strings, essentially dealing with it the same way that the bug appeared in the first place. I've adjusted them to the desired behavior.

Copy link
Collaborator

@WoH WoH left a comment

Choose a reason for hiding this comment

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

Small changes, but mainly one request: There's a similar enum handling happening for OpenAPI 2 here. Please make sure we don't diverge when we don't have to.

@WoH
Copy link
Collaborator

WoH commented Nov 1, 2021

Please rebase so the CI fixes from the main branch are applied.

@FlorianRappl
Copy link
Contributor Author

FlorianRappl commented Nov 1, 2021

Small changes, but mainly one request: There's a similar enum handling happening for OpenAPI 2 here. Please make sure we don't diverge when we don't have to.

I am not sure about OAS 2 - the issue was placed for OAS 3 and I am not sure how the enum handling is supposed to look for OAS 2. If its similar then we can apply it there.

Please rebase so the CI fixes from the main branch are applied.

Done.

@WoH
Copy link
Collaborator

WoH commented Nov 1, 2021

I am not sure about OAS 2 - the issue was placed for OAS 3 and I am not sure how the enum handling is supposed to look for OAS 2. If its similar then we can apply it there.

Should follow the same logic

@FlorianRappl
Copy link
Contributor Author

Alright, great - all done @WoH ! Thanks for the suggestions 🚀

@WoH WoH merged commit 596e163 into lukeautry:master Nov 1, 2021
@FlorianRappl FlorianRappl deleted the fix/enum-number-#1068 branch November 5, 2021 16:03
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