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

Put short-circuiting code in one place #1635

Merged
merged 3 commits into from
Jul 14, 2021
Merged

Put short-circuiting code in one place #1635

merged 3 commits into from
Jul 14, 2021

Conversation

Porges
Copy link
Member

@Porges Porges commented Jul 8, 2021

A little tidy-up.

@Porges Porges changed the title Handle another case in allOf Put short-circuiting code in one place Jul 8, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2021

Codecov Report

Merging #1635 (39490d9) into master (744f90b) will increase coverage by 0.21%.
The diff coverage is 67.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1635      +/-   ##
==========================================
+ Coverage   66.63%   66.85%   +0.21%     
==========================================
  Files         204      204              
  Lines       14760    14788      +28     
==========================================
+ Hits         9835     9886      +51     
+ Misses       4169     4145      -24     
- Partials      756      757       +1     
Impacted Files Coverage Δ
hack/generator/pkg/astmodel/validated_type.go 77.06% <0.00%> (-4.49%) ⬇️
...gen/pipeline/convert_allof_and_oneof_to_objects.go 70.70% <0.00%> (ø)
...k/generator/pkg/codegen/pipeline/status_augment.go 27.83% <0.00%> (+2.12%) ⬆️
hack/generator/pkg/astmodel/map_type.go 85.41% <75.00%> (-3.48%) ⬇️
hack/generator/pkg/astmodel/array_type.go 87.87% <100.00%> (+2.69%) ⬆️
hack/generator/pkg/astmodel/flagged_type.go 95.23% <100.00%> (+0.11%) ⬆️
hack/generator/pkg/astmodel/optional_type.go 92.98% <100.00%> (+6.31%) ⬆️
hack/generator/pkg/astmodel/resource_type.go 79.32% <100.00%> (-0.06%) ⬇️
hack/generator/pkg/astmodel/type_visitor.go 53.52% <100.00%> (-1.59%) ⬇️
hack/generated/pkg/genruntime/resolver.go 83.58% <0.00%> (-2.99%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 744f90b...39490d9. Read the comment docs.

Copy link
Member

@babbageclunk babbageclunk left a comment

Choose a reason for hiding this comment

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

Looks great! This way around makes it a much nicer idiom in the codebase.

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

Overall, I like the approach of ensuring we share more immutable values, reducing memory churn. Do any of your new withers need to use an internal copy() func to follow our prior conventions?

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Giphy

@Porges Porges merged commit 668c02b into master Jul 14, 2021
@Porges Porges deleted the short-circuiting branch July 14, 2021 00:14
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.

5 participants