Skip to content

Conversation

@dsa0x
Copy link
Member

@dsa0x dsa0x commented Apr 1, 2025

Applies some new collection features, specificially copying of maps and slices.

Fixes #

Target Release

1.12.x

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@dsa0x dsa0x added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Apr 1, 2025
for k, v := range existing {
ret[k] = v
}
maps.Copy(ret, existing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should all be maps.Clone when we're allocating the empty map just above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did make some of them maps.Clone, but there was a single situation where that led to test failures because maps.Clone returns a nil object when the object-to-be-cloned is nil, but the test expected a zero-length map. That was what made me go all defensive and use Copy instead of Clone.

Perhaps it is sufficient to rely on tests and use maps.Clone anywhere possible? I got nervous about breaking things due to those changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's a good point! Some of these were probably counting on never returning a nil map. Anything that feeds into cty.ObjectVal is an obvious candidate, which would be map[string]cty.Value, though I only see one of those in this diff. Maybe if you can easily prove the code is fine passing through a nil map you could use Clone, and leave a comment why it's Copy on any you're unsure about? I would just want at least a comment bc someone may eventually come through and make the same change again later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments to the sections, where the map is returned, and thus may be used beyond the current scope.

For the ones where the map is immediately written to in the current scope, I find it redundant to add the comment, and expecting that one would ensure to allocate a map before attempting to write to it.

@dsa0x dsa0x requested a review from jbardin April 2, 2025 10:06
@dsa0x dsa0x force-pushed the sams/1-24-maps-features branch from 678f1a5 to bfb28c4 Compare April 2, 2025 10:07
@dsa0x dsa0x marked this pull request as ready for review April 2, 2025 10:27
@dsa0x dsa0x requested review from a team as code owners April 2, 2025 10:27
jbardin
jbardin previously approved these changes Apr 2, 2025
@dsa0x
Copy link
Member Author

dsa0x commented Apr 2, 2025

I had to fix a merge conflict @jbardin

@dsa0x dsa0x merged commit c0a7ff2 into main Apr 2, 2025
8 checks passed
@dsa0x dsa0x deleted the sams/1-24-maps-features branch April 2, 2025 14:20
@dsa0x dsa0x restored the sams/1-24-maps-features branch April 2, 2025 14:20
@dsa0x dsa0x mentioned this pull request Apr 2, 2025
2 tasks
dsa0x added a commit that referenced this pull request Apr 3, 2025
@dsa0x dsa0x deleted the sams/1-24-maps-features branch April 3, 2025 08:41
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2025

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants