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

Change Properties to a map #1655

Merged
merged 9 commits into from
Jul 22, 2021
Merged

Change Properties to a map #1655

merged 9 commits into from
Jul 22, 2021

Conversation

theunrepentantgeek
Copy link
Member

@theunrepentantgeek theunrepentantgeek commented Jul 20, 2021

What this PR does / why we need it:

Changes the Properties() method exposed by ObjectType and ResourceType to return a (strongly typed) map instead of a slice.

Most of the consumers of the method don't care about the ordering, but they're paying the price of sorting.

Introducing the type PropertySet allows for easy conversion to a slice for the few consumers that care (by calling AsSlice(), while allowing random access for other consumers.

Note that this changes the order of propeties returned from ResourceType - they're now alphabetical (Spec/Status) instead of hard coded (Status/Spec); this has resulted in minor changes to a few golden files.

Special notes for your reviewer:

The Properties() method is called a lot of times (67 references) and I haven't exahustively reviewed all of them. Instead, I've relied on the compiler, linter, and our tests, to find cases where updates are needed.

How does this PR make you feel:
gif

If applicable:

  • this PR contains tests

@theunrepentantgeek theunrepentantgeek self-assigned this Jul 20, 2021
@theunrepentantgeek theunrepentantgeek added this to the codegen-alpha-1 milestone Jul 20, 2021
@theunrepentantgeek theunrepentantgeek force-pushed the feature/propertyset branch 2 times, most recently from ba88b90 to 6a873c3 Compare July 20, 2021 22:39
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2021

Codecov Report

Merging #1655 (e92d643) into master (02973a1) will increase coverage by 0.56%.
The diff coverage is 74.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1655      +/-   ##
==========================================
+ Coverage   67.08%   67.65%   +0.56%     
==========================================
  Files         205      211       +6     
  Lines       14840    15179     +339     
==========================================
+ Hits         9956    10269     +313     
- Misses       4126     4146      +20     
- Partials      758      764       +6     
Impacted Files Coverage Δ
hack/generator/cmd/gen_kustomize.go 0.00% <0.00%> (ø)
hack/generator/cmd/gen_types.go 0.00% <0.00%> (ø)
hack/generator/cmd/root.go 0.00% <0.00%> (ø)
.../generator/pkg/astmodel/code_generation_context.go 46.87% <0.00%> (ø)
...erator/pkg/astmodel/conversion_function_builder.go 69.10% <0.00%> (-0.45%) ⬇️
hack/generator/pkg/astmodel/file_definition.go 91.53% <0.00%> (ø)
hack/generator/pkg/astmodel/package_definition.go 30.00% <0.00%> (ø)
hack/generator/pkg/astmodel/package_import.go 89.47% <0.00%> (ø)
hack/generator/pkg/astmodel/property_container.go 75.00% <ø> (ø)
...ack/generator/pkg/astmodel/test_file_definition.go 0.00% <0.00%> (ø)
... and 76 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 1e5b3d2...e92d643. Read the comment docs.

@theunrepentantgeek theunrepentantgeek merged commit f20673d into master Jul 22, 2021
@theunrepentantgeek theunrepentantgeek deleted the feature/propertyset branch July 22, 2021 23:58
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