search

zclconf / go-cty

A type system for dynamic values in Go applications
MIT License
338 stars 70 forks source link

feat: added CompactMap function and tests. #149

Closed aidanmelen closed 1 year ago

aidanmelen commented 1 year ago

feat: added CompactMap function and tests. fix: added missing test for the Compact function.

aidanmelen commented 1 year ago

Relates to: https://github.com/hashicorp/terraform/issues/31823

apparentlymart commented 1 year ago

Hi @aidanmelen! Thanks for working on this.

It doesn't look like the Terraform team has accepted that proposal yet, and if they do I'd probably suggest implementing the function in the Terraform codebase or in HashiCorp's shared library of functions instead to start.

I have come to regret accepting the upstreaming of CompactFunc from Terraform because it implement's Terraform's oddity of treating empty string as representing absence of as string, instead of null. It would be a breaking change to fix that now, but I'd prefer not to continue that mistake with another function.

I might consider including a version of this which eliminates elements that are null, rather than empty string, but even then I think I'd prefer to see this function proven in a downstream application first before accepting it upstream, because the library of functions here is already a bit too large for me to maintain well and so I'm trying to be selective about accepting only new functions that seem broadly useful in many different applications.

Since the Terraform team hasn't yet accepted the proposal I don't know that they'd review a PR for this in that repository yet either, but I think that repository would be the best place to discuss this to start and then the Terraform team can decide whether this is something they'd accept into Terraform, and if it sees wide use there (assuming it were implemented in a form that only eliminates null elements, not empty string) then I'd consider accepting it upstream.

Thanks again!

aidanmelen commented 1 year ago

As per your suggestion, I will plan to move this pull request with the null only prune logic to the go-cty-func repo if you think that is best. I initially chose this repo because it contained the code for the compact list function. Thank you for circling back to this PR!