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

Change a bunch of code to use generics #3078

Merged
merged 6 commits into from
Jul 11, 2022
Merged

Change a bunch of code to use generics #3078

merged 6 commits into from
Jul 11, 2022

Conversation

dnr
Copy link
Member

@dnr dnr commented Jul 8, 2022

What changed?

  • Move common.Min/Max* and functions and a few others to new common/util package to prevent circular imports.
  • Make Min/Max generic, except for Min/MaxTime, which need to use special time functions.
  • Change a bunch of code to use slices.Clone, slices.Contains, maps.Clone, maps.Copy, maps.Keys, and maps.Values from golang.org/x/exp.
  • Add generic common.CloneProto and switch users of proto.Clone
  • service/matching/tasks.go can just use util.SortSlice
  • Simplified surrounding code if it made sense.

I tried to keep this to only simple noncontroversial changes.

Why?
Clearer code

How did you test it?
Existing unit tests and integration tests

Potential risks
typos

Is hotfix candidate?
no

@dnr dnr requested a review from a team as a code owner July 8, 2022 22:43
@dnr
Copy link
Member Author

dnr commented Jul 11, 2022

Some of the shard controller tests were flaky on my machine and it was hard to check the changes in there. I added a few sleeps to make them more reliable for now, but we should fix these with proper synchronization later. If you want I can merge without the sleeps

@dnr dnr merged commit 9d632d7 into temporalio:master Jul 11, 2022
@dnr dnr deleted the generics1 branch July 11, 2022 20:00
dnr added a commit to dnr/temporal that referenced this pull request Jul 11, 2022
dnr added a commit that referenced this pull request Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants