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

[sdk/dotnet] Improve collection initializers #8498

Closed
wants to merge 5 commits into from
Closed

[sdk/dotnet] Improve collection initializers #8498

wants to merge 5 commits into from

Conversation

gitfool
Copy link
Contributor

@gitfool gitfool commented Nov 26, 2021 •
edited
Loading

Fixes #8497. See the InputMapCollectionInitializers and InputListCollectionInitializers tests for examples of flexibility and usability.

@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

1 similar comment
@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@gitfool gitfool changed the title Improve collection initializers [sdk/dotnet] Improve collection initializers Nov 26, 2021
@github-actions
Copy link

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@gitfool
Copy link
Contributor Author

gitfool commented Nov 29, 2021

@justinvp I've applied your suggested changes.

@justinvp
Copy link
Member

justinvp commented Dec 1, 2021

/run-acceptance-tests

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

Please view the results of the PR Build + Acceptance Tests Run Here

@justinvp
Copy link
Member

justinvp commented Dec 1, 2021

Thanks, @gitfool!

I don't know what the original intention was behind the use of params in InputList<T>.Add. Adding a collection to an existing collection via Add (and therefore also collection initializers) is a little unusual (I'd expect to do that through an AddRange method). That said, I do see how being able to add various types of collections as part of initializing a collection is useful for Pulumi in particular (thanks for the examples in the tests that demonstrate this), and we already allow doing this through the Add(params), so I'm on board with these changes. cc: @Frassle and @mikhailshilkov in case they have any other thoughts.

@gitfool
Copy link
Contributor Author

gitfool commented Dec 1, 2021 •
edited
Loading

Note: I do have a practical case in mind that lead me to submitting this - succinctly combining dependencies in their various forms.

FWIW, it's a shame we couldn't make a breaking change to remove the params specification since it doesn't work as intended. It calls Add multiple times for each param, rather than once for all params, and given all the manipulation to combine outputs, fewer calls would be better. (Removing the params doesn't make it better in that way but does remove any confusion / expectation that it's only called once.)

@justinvp
Copy link
Member

justinvp commented Dec 1, 2021

/run-acceptance-tests

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@github-actions
Copy link

github-actions bot commented Dec 1, 2021

Please view the results of the PR Build + Acceptance Tests Run Here

1 similar comment
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Please view the results of the PR Build + Acceptance Tests Run Here

Frassle
Frassle reviewed Dec 6, 2021
View reviewed changes
sdk/dotnet/Pulumi/Core/InputList.cs Outdated Show resolved Hide resolved
sdk/dotnet/Pulumi/Core/Output.cs Outdated Show resolved Hide resolved
sdk/dotnet/Pulumi.Tests/Core/InputTests.cs Show resolved Hide resolved
sdk/dotnet/Pulumi/Core/InputList.cs Show resolved Hide resolved
sdk/dotnet/Pulumi/Core/InputMap.cs Show resolved Hide resolved
@t0yv0
Copy link
Member

t0yv0 commented Dec 6, 2021

@justinvp checking in on this, can we merge this as-is or should take some changes first? I'm on board with the changes for the functionality, I don't index on the C# being idiomatic as highly, so long as this is discoverable.

@github-actions
Copy link

github-actions bot commented Dec 6, 2021

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@github-actions
Copy link

github-actions bot commented Dec 6, 2021

PR is now waiting for a maintainer to take action.

Note for the maintainer: Commands available:

  • /run-acceptance-tests - used to test run the acceptance tests for the project
  • /run-codegen - used to test the Pull Request against downstream codegen
  • /run-docs-gen - used to test the Pull Request against documentation generation
  • /run-containers - used to test the Pull Request against the containers tests

@gitfool
Copy link
Contributor Author

gitfool commented Dec 6, 2021

I made some follow-up changes to hopefully address concerns but still augment the collection initializers, albeit in a non-standard way, bearing in mind the extensive use of these collections.

@t0yv0
Copy link
Member

t0yv0 commented Dec 8, 2021

/run-acceptance-tests

@github-actions
Copy link

github-actions bot commented Dec 8, 2021

Please view the results of the PR Build + Acceptance Tests Run Here

@gitfool
Copy link
Contributor Author

gitfool commented Dec 9, 2021

@t0yv0 the many acceptance test failures look unrelated to this change.

@t0yv0 t0yv0 mentioned this pull request Dec 10, 2021
Merged
3 tasks
@ie-zero ie-zero mentioned this pull request Dec 16, 2021
Closed
@t0yv0
Copy link
Member

t0yv0 commented Dec 20, 2021

Thank you for this contribution 🎉

It got accepted via #8582

@t0yv0 t0yv0 closed this Dec 20, 2021
@gitfool gitfool deleted the collection-initializers branch December 20, 2021 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Frassle Frassle Frassle left review comments

@mikhailshilkov mikhailshilkov mikhailshilkov left review comments

@justinvp justinvp justinvp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[sdk/dotnet] Improve collection initializers
5 participants
@gitfool @justinvp @t0yv0 @Frassle @mikhailshilkov