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

all: simplify and clean up #19126

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Conversation

callthingsoff
Copy link
Contributor

@callthingsoff callthingsoff commented Jan 4, 2025

This patch modernizes the for-range-loop code to copy a map with
"maps.Clone" and "maps.Copy", also eliminates "copyFloats" with
"slices.Clone".

Also simplify "aggSort" and "sortMap" with slices and maps functions.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@k8s-ci-robot
Copy link

Hi @callthingsoff. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link

codecov bot commented Jan 4, 2025

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.80%. Comparing base (70a1726) to head (2415c82).
Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
server/proxy/grpcproxy/adapter/chan_stream.go 0.00% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
pkg/featuregate/feature_gate.go 88.35% <100.00%> (+0.69%) ⬆️
pkg/report/report.go 94.02% <100.00%> (-0.22%) ⬇️
server/proxy/grpcproxy/adapter/chan_stream.go 55.84% <0.00%> (+0.71%) ⬆️

... and 28 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19126      +/-   ##
==========================================
- Coverage   68.86%   68.80%   -0.06%     
==========================================
  Files         420      420              
  Lines       35640    35626      -14     
==========================================
- Hits        24543    24512      -31     
- Misses       9679     9687       +8     
- Partials     1418     1427       +9     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70a1726...2415c82. Read the comment docs.

@ivanvc
Copy link
Member

ivanvc commented Jan 6, 2025

/ok-to-test

serathius
serathius previously approved these changes Jan 7, 2025
@ivanvc
Copy link
Member

ivanvc commented Jan 8, 2025

Hi @callthingsoff, this looks like a good clean up, but it does have some test failures. Could you address them? Thanks.

@callthingsoff callthingsoff force-pushed the modernize_map_op branch 2 times, most recently from 5064f72 to b658719 Compare January 8, 2025 01:10
@callthingsoff
Copy link
Contributor Author

Hi @callthingsoff, this looks like a good clean up, but it does have some test failures. Could you address them? Thanks.

Sure, maps.Clone can’t ensure the result is non-nil, I replaced all the possible nil-map-Clone with maps.Copy. PTAL.

This patch modernizes the for-range-loop code to copy a map with
"maps.Clone" and "maps.Copy", also eliminates "copyFloats" with
"slices.Clone".

Also simplify "aggSort" and "sortMap" with slices and maps functions.

Signed-off-by: Jes Cok <[email protected]>
@callthingsoff
Copy link
Contributor Author

/retest-required

Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @callthingsoff.

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks @callthingsoff

@serathius
Copy link
Member

/ok-to-test

@callthingsoff
Copy link
Contributor Author

Thanks, it’s already labeled ok-to-test, and the tests have passed.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the cleanup!

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, callthingsoff, ivanvc, jmhbnz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahrtr ahrtr merged commit 6277dbe into etcd-io:main Jan 9, 2025
35 checks passed
@callthingsoff callthingsoff deleted the modernize_map_op branch January 9, 2025 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants