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

Add type constraints for Map and Array fields #43

Merged
merged 4 commits into from
Nov 13, 2023
Merged

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Oct 26, 2023

Summary

By adding type constraints for Map and Array fields, it's ensured at compile time that no non-array or non-map is passed to these methods.

This is the first introduction of generics in this codebase. I hope it's allowed.

Ticket Link

None

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Oct 26, 2023
@hanzei hanzei requested a review from wiggin77 October 26, 2023 11:04
@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Unit Test Results

71 tests  +2   71 ✔️ +2   24s ⏱️ ±0s
10 suites ±0     0 💤 ±0 
  1 files   ±0     0 ±0 

Results for commit 07edba1. ± Comparison against base commit e0a61b2.

♻️ This comment has been updated with latest results.

@wiggin77
Copy link
Member

@hanzei normally an API signature change would be a breaking change requiring a major version bump. I think this is closer to something like changing interface{} to any. The only exception being any case where a non-map or slice is currently passed which won't compile now.

Thoughts on whether we need a full version bump? I've not worked much with generics in Go yet.

@hanzei
Copy link
Contributor Author

hanzei commented Nov 1, 2023

I've added a couple of tests in 289a135 and 07edba1 to ensure the code complies with any combination I could think of. Yes, existing code will fail to compile with a non-map or non-slice type.

Error caused by the changes in this PR are "good" errors IMO.

@hanzei
Copy link
Contributor Author

hanzei commented Nov 13, 2023

@wiggin77 Can we merge this PR?

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Nov 13, 2023
@hanzei hanzei merged commit bcc267c into master Nov 13, 2023
2 checks passed
@hanzei hanzei deleted the typesafe-map-array branch November 13, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants