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

fix: Allow more specific object keys #182

Merged
merged 1 commit into from
Nov 12, 2024
Merged

fix: Allow more specific object keys #182

merged 1 commit into from
Nov 12, 2024
+684 −2

Conversation

nickvergessen
Copy link
Member

No description provided.

@nickvergessen nickvergessen added the bug Something isn't working label Nov 11, 2024
@nickvergessen nickvergessen self-assigned this Nov 11, 2024
@nickvergessen nickvergessen mentioned this pull request Nov 11, 2024
Merged
1 task
provokateurin
provokateurin requested changes Nov 11, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

The error message should probably be improved as well, to explain which types can be used besides string.

src/OpenApiType.php Outdated Show resolved Hide resolved
@nickvergessen nickvergessen force-pushed the bugfix/noid/allow-more-specific-object-keys branch from befa43b to c5e81e9 Compare November 12, 2024 07:33
provokateurin
provokateurin requested changes Nov 12, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Please add some tests for this.

You could also re-use the same array for the check and the error message to avoid that they ever drift apart, but it's not that important.

@nickvergessen nickvergessen force-pushed the bugfix/noid/allow-more-specific-object-keys branch from c5e81e9 to ce0c021 Compare November 12, 2024 07:53
@nickvergessen
Copy link
Member Author

So merge and release, so notifications can go green again? :D

@nickvergessen nickvergessen merged commit 2587f18 into main Nov 12, 2024
8 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@provokateurin provokateurin provokateurin approved these changes

Assignees

@nickvergessen nickvergessen

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nickvergessen @provokateurin