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

Simplify Field API using generics #44

Merged
merged 9 commits into from
Nov 14, 2023
Merged

Simplify Field API using generics #44

merged 9 commits into from
Nov 14, 2023

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Nov 10, 2023

Summary

Internally, logr threads typed that allow Type conversions to "higher" types the same. E.g. an int32get converted into an int64 anyway.

By adding generics field functions for each "class" of types, the API becomes simpler to read and use. No type conversions are needed on the callers' side.

All field functions also allow the usage of type definition without type conversions from the caller.

To summarize:

  • Int replaces Int32 and Int64
  • Uint replaces Uint32 and Uint64
  • Float replaces Float32 and Float64

I thought about consolidating the fields even further, but that adds problems with gojay.

Based on top of #43

Ticket Link

None

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Nov 10, 2023
@hanzei hanzei requested a review from wiggin77 November 10, 2023 09:00
@hanzei
Copy link
Contributor Author

hanzei commented Nov 10, 2023

Are the different FieldTypes even needed at this point? Or can they get removed?

Copy link

github-actions bot commented Nov 10, 2023

Unit Test Results

81 tests  +10   81 ✔️ +10   23s ⏱️ -1s
10 suites ±  0     0 💤 ±  0 
  1 files   ±  0     0 ±  0 

Results for commit 1ea2bc0. ± Comparison against base commit bcc267c.

♻️ This comment has been updated with latest results.

fieldapi_test.go Outdated Show resolved Hide resolved
fieldapi.go Outdated Show resolved Hide resolved
fieldapi.go Outdated Show resolved Hide resolved
fieldapi_test.go Outdated Show resolved Hide resolved
fieldapi_test.go Outdated Show resolved Hide resolved
fieldapi_test.go Outdated Show resolved Hide resolved
fieldapi_test.go Outdated Show resolved Hide resolved
fieldapi_test.go Outdated Show resolved Hide resolved
Copy link
Member

@wiggin77 wiggin77 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Some minor typos to fix.

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed 2: Dev Review Requires review by a core committer labels Nov 13, 2023
@hanzei
Copy link
Contributor Author

hanzei commented Nov 13, 2023

Thanks for the review @wiggin77 👍 Can we merge the PR?

Base automatically changed from typesafe-map-array to master November 13, 2023 13:28
@hanzei hanzei removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Nov 13, 2023
@hanzei hanzei merged commit a02d145 into master Nov 14, 2023
2 checks passed
@hanzei hanzei deleted the simplify-fields branch November 14, 2023 08:16
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