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

Field Merging[2/x] Compute MergedSelections w/MergingStrategy #307

Closed
wants to merge 4 commits into from
Closed

Field Merging[2/x] Compute MergedSelections w/MergingStrategy #307

wants to merge 4 commits into from

Conversation

AnthonyMDev
Copy link
Contributor

@AnthonyMDev AnthonyMDev commented Mar 21, 2024 •
edited
Loading

TL;DR

Updated the logic for merging selections in ComputedSelectionSet.

What changed?

  • Implemented a new MergedSelections logic to handle different merging strategies.

How to test?

Unit tests will be written before this PR is marked ready for review.

Why make this change?

To improve the handling of merged selections in ComputedSelectionSet based on different merging strategies.

@AnthonyMDev Graphite App
Copy link
Contributor Author

AnthonyMDev commented Mar 21, 2024 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @AnthonyMDev and the rest of your teammates on Graphite Graphite

@AnthonyMDev AnthonyMDev mentioned this pull request Mar 21, 2024
Closed
@AnthonyMDev AnthonyMDev force-pushed the 03-21-field_merging_2_x_compute_mergedselections_w_mergingstrategy branch from 73b01cd to c4eae7f Compare March 22, 2024 01:13
@AnthonyMDev AnthonyMDev force-pushed the 03-20-field_merging_1_x_configuration_options branch from eec8762 to f9cdce4 Compare March 22, 2024 01:21
@AnthonyMDev AnthonyMDev force-pushed the 03-21-field_merging_2_x_compute_mergedselections_w_mergingstrategy branch from c4eae7f to fcc173f Compare March 22, 2024 01:21
@AnthonyMDev AnthonyMDev mentioned this pull request Mar 22, 2024
Merged
@AnthonyMDev AnthonyMDev force-pushed the 03-20-field_merging_1_x_configuration_options branch from f9cdce4 to 88084be Compare March 25, 2024 23:11
@AnthonyMDev AnthonyMDev force-pushed the 03-21-field_merging_2_x_compute_mergedselections_w_mergingstrategy branch from fcc173f to 09edc8e Compare March 25, 2024 23:11
@AnthonyMDev AnthonyMDev mentioned this pull request Mar 25, 2024
Closed
@tahirmt
Copy link
Contributor

tahirmt commented Mar 26, 2024

@AnthonyMDev I ran this branch without field merging and I'm still seeing merged fields. I reproduced it on a sample project for easy review https://github.com/tahirmt/apollo-codegen-demo. Maybe I missed something but I think I got everything set up correctly.

Here fieldMerging is set to none
https://github.com/tahirmt/apollo-codegen-demo/blob/98b3390ecb0b35e3c4939176656f44442a90aab0/Sources/codegen/Command.swift#L31

And here is the generated query still showing merged fields
https://github.com/tahirmt/apollo-codegen-demo/blob/98b3390ecb0b35e3c4939176656f44442a90aab0/API/Sources/Operations/Queries/NodeQuery.graphql.swift#L60

Let me know if I missed anything

@AnthonyMDev
Copy link
Contributor Author

I made a very silly oversight yesterday. While the field merging strategies work properly, they are not actually affecting the generated templates yet. I'm going to get that sorted out today and will have another PR up with this working soon!
Sorry for the confusion @tahirmt

@tahirmt
Copy link
Contributor

tahirmt commented Mar 26, 2024

Ah no worries. I was just too excited to try out.

@AnthonyMDev AnthonyMDev force-pushed the 03-21-field_merging_2_x_compute_mergedselections_w_mergingstrategy branch from 09edc8e to 561c9c4 Compare March 26, 2024 23:18
@AnthonyMDev AnthonyMDev mentioned this pull request Mar 26, 2024
Closed
@AnthonyMDev AnthonyMDev marked this pull request as ready for review March 26, 2024 23:20
@AnthonyMDev AnthonyMDev force-pushed the 03-20-field_merging_1_x_configuration_options branch from 88084be to 5748c50 Compare April 24, 2024 19:28
@AnthonyMDev AnthonyMDev force-pushed the 03-21-field_merging_2_x_compute_mergedselections_w_mergingstrategy branch from 561c9c4 to 70ea639 Compare April 24, 2024 19:28
This was referenced Apr 24, 2024
Closed
Closed
@AnthonyMDev AnthonyMDev force-pushed the 03-21-field_merging_2_x_compute_mergedselections_w_mergingstrategy branch from 70ea639 to 36dc2e6 Compare May 1, 2024 01:09
@AnthonyMDev AnthonyMDev mentioned this pull request May 1, 2024
Closed
@AnthonyMDev AnthonyMDev force-pushed the 03-20-field_merging_1_x_configuration_options branch from 5748c50 to 86fec35 Compare May 1, 2024 01:20
@AnthonyMDev AnthonyMDev force-pushed the 03-21-field_merging_2_x_compute_mergedselections_w_mergingstrategy branch from 36dc2e6 to 1252393 Compare May 1, 2024 01:20
@AnthonyMDev AnthonyMDev force-pushed the 03-20-field_merging_1_x_configuration_options branch 2 times, most recently from 3cbbd5e to 41f58c4 Compare July 16, 2024 19:51
@AnthonyMDev
Copy link
Contributor Author

Closing. This is replaced by #431

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AnthonyMDev @tahirmt