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

Fix index method name collisions #2335

Merged
merged 13 commits into from
Jun 10, 2022
Merged

Conversation

theunrepentantgeek
Copy link
Member

@theunrepentantgeek theunrepentantgeek commented Jun 9, 2022

What this PR does / why we need it:

We are now encountering collisions with our generated index method names, caused by having multiple properties nested (via different paths) with the same name.

This PR uses parts of the full property path to the property to avoid collisions, as we discussed offline.

Closes #2332

Special notes for your reviewer:

Takes the preexisting indexFunctionBuilderContext type and converts it into a full abstraction for a property chain. The implementation is changed so that chains form a tree of (nearly) immutable references, both to keep memory use down and to allow disambiguation.

How does this PR make you feel:
gif

If applicable:

  • this PR contains tests

@theunrepentantgeek theunrepentantgeek added this to the v2.0.0-beta.1 milestone Jun 9, 2022
@theunrepentantgeek theunrepentantgeek self-assigned this Jun 9, 2022
@theunrepentantgeek theunrepentantgeek changed the title Rename indexFunctionBuilderContext to propertyChain Fix index method name collisions Jun 9, 2022
@theunrepentantgeek theunrepentantgeek enabled auto-merge (squash) June 9, 2022 01:27
@codecov-commenter
Copy link

Codecov Report

Merging #2335 (a158fa5) into main (9603436) will increase coverage by 0.00%.
The diff coverage is 64.17%.

❗ Current head a158fa5 differs from pull request most recent head cbe6226. Consider uploading reports for the commit cbe6226 to get more accurate results

@@           Coverage Diff           @@
##             main    #2335   +/-   ##
=======================================
  Coverage   53.21%   53.21%           
=======================================
  Files         853      853           
  Lines      259188   259246   +58     
=======================================
+ Hits       137930   137964   +34     
- Misses     101869   101887   +18     
- Partials    19389    19395    +6     
Impacted Files Coverage Δ
...n/pipeline/export_controller_type_registrations.go 36.49% <64.17%> (+28.88%) ⬆️
...ernal/testcommon/error_translating_roundtripper.go 11.53% <0.00%> (-50.00%) ⬇️
v2/internal/metrics/arm_client_metrics.go 84.00% <0.00%> (-8.00%) ⬇️
v2/internal/genericarmclient/generic_client.go 77.35% <0.00%> (-1.89%) ⬇️
...ilers/arm/azure_generic_arm_reconciler_instance.go 73.73% <0.00%> (-0.60%) ⬇️
v2/internal/testcommon/kube_per_test_context.go 87.28% <0.00%> (+0.05%) ⬆️
...rmysql/v1beta20210501/flexible_server_types_gen.go 60.16% <0.00%> (+0.74%) ⬆️
v2/internal/reconcilers/common.go 82.75% <0.00%> (+2.75%) ⬆️

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 954eb05...cbe6226. Read the comment docs.

@matthchr
Copy link
Member

matthchr commented Jun 9, 2022

I've also made changes here to resolve the issue raised by @majguo in #2330, and I suspect our changes are going to conflict. Let's sync up and determine what order to merge them in @theunrepentantgeek.

@theunrepentantgeek theunrepentantgeek changed the title Fix index method name collisions Fix index method name collisions Jun 9, 2022
@theunrepentantgeek theunrepentantgeek enabled auto-merge (squash) June 10, 2022 02:25
@theunrepentantgeek theunrepentantgeek merged commit 81392c0 into main Jun 10, 2022
@theunrepentantgeek theunrepentantgeek deleted the improve/index-naming branch June 10, 2022 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Index methods generating with duplicate names
4 participants