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: Support allOf additionalProperties merging if both are the equal #1330

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

abemedia
Copy link
Contributor

We have a bunch of objects we merge using allOf where both have the exact same value for additionalProperties, however we get the following error:

Feature "allOf additionalProperties merging" is not implemented yet.

This PR adds support for merging if both items are equal.

@abemedia abemedia changed the title feat: Support additionalProperties merging if both are the equal feat: Support allOf additionalProperties merging if both are the equal Oct 14, 2024
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 72.73%. Comparing base (e2dc75e) to head (887f69a).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
gen/schema_gen_sum.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1330      +/-   ##
==========================================
- Coverage   72.74%   72.73%   -0.02%     
==========================================
  Files         190      190              
  Lines       13096    13101       +5     
==========================================
+ Hits         9527     9529       +2     
- Misses       3017     3021       +4     
+ Partials      552      551       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gen/schema_gen_sum.go Outdated Show resolved Hide resolved
Copy link
Member

@tdakkota tdakkota left a comment

Choose a reason for hiding this comment

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

LGTM in general, but probably we should merge additionalProperties schemas, if we can.

@abemedia
Copy link
Contributor Author

@tdakkota have amended to merge the schemas.

@tdakkota tdakkota enabled auto-merge October 14, 2024 14:23
@tdakkota tdakkota merged commit b72b870 into ogen-go:main Oct 14, 2024
13 checks passed
@abemedia abemedia deleted the patch-2 branch October 14, 2024 16:38
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