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

Added to package to convert to pointers #18

Merged
merged 4 commits into from
Jul 26, 2022
Merged

Conversation

ItalyPaleAle
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle commented Jul 26, 2022

Description

This added a new package to dapr/kit called ptr which contains a method to convert any value to a pointer, based on generics. (The code for that function is copied from the Azure SDK, because of the Go's philosophy "a little copying is better than a little dependency" - the MIT license permits that freely)

Currently Dapr was using agrea/ptr for that purpose, which didn't use generics so it was limited to the types that were explicitly supported. Since we use Go 1.18 now and can rely on generics, the new approach is more convenient.

Also, this PR updates dependencies and the linter.

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests

Also updated deps and removed agrea/ptr

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle requested a review from a team as a code owner July 26, 2022 01:06
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2022

Codecov Report

Merging #18 (1fa2272) into main (15a34b1) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #18      +/-   ##
==========================================
+ Coverage   92.63%   92.70%   +0.07%     
==========================================
  Files           7        8       +1     
  Lines         285      288       +3     
==========================================
+ Hits          264      267       +3     
  Misses         15       15              
  Partials        6        6              
Impacted Files Coverage Δ
logger/dapr_logger.go 90.90% <ø> (ø)
config/decode.go 95.45% <100.00%> (-0.06%) ⬇️
logger/options.go 80.64% <100.00%> (ø)
ptr/of.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

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

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
Copy link
Member

@pkedy pkedy left a comment

Choose a reason for hiding this comment

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

@ItalyPaleAle I am supportive of this, but I have a question before merging. There are only a handful of replacements that could be made in dapr/dapr. Where else could this be beneficial?

@ItalyPaleAle
Copy link
Contributor Author

@pkedy this is one example in a PR where I needed to use the method from the Azure SDK because I needed a pointer to a time.Duration: https://github.com/dapr/dapr/pull/4903/files#diff-64faf11a53da287dad97fd63381370362fea01d0d9f9f3a8f4812c4a52b873c2R101

Not a critical thing, of course. During development, I needed to change those values without changing the other tests (then reverted so the values are the same, so technically I could just revert the change and this wouldn't be needed at all - but it could still be useful).

@pkedy pkedy merged commit 247f04d into dapr:main Jul 26, 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.

3 participants