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

Type safe stashing of context values #546

Merged
merged 2 commits into from
Sep 20, 2024
Merged

Conversation

scothis
Copy link
Contributor

@scothis scothis commented Sep 19, 2024

Introducing a Stasher which is a convenience helper for interacting with typed values for a single stash key. The stasher itself is generic and handles casting retrieved values to the desired type.

There are three different ways to handle the value not existing or being of the wrong type:

  • RetrieveOrDie panics if a value of the desired type is not found
  • RetrieveOrError errors if a value of the desired type is not found
  • RetrieveOrEmpty falls back to the empty value for the type

Introducing a Stasher which is a convenience helper for interacting with
typed values for a single stash key. The stasher itself is generic and
handles casting retrieved values to the desired type.

There are three different ways to handle the value not existing or being
of the wrong type:

- RetrieveOrDie panics if a value of the desired type is not found
- RetrieveOrError errors if a value of the desired type is not found
- RetrieveOrEmpty falls back to the empty value for the type

Signed-off-by: Scott Andrews <scott@andrews.me>
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.52%. Comparing base (435c49f) to head (5dcfc6a).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #546      +/-   ##
==========================================
+ Coverage   60.34%   60.52%   +0.17%     
==========================================
  Files          32       32              
  Lines        2867     2880      +13     
==========================================
+ Hits         1730     1743      +13     
  Misses       1043     1043              
  Partials       94       94              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mamachanko mamachanko left a comment

Choose a reason for hiding this comment

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

nice!

Comment on lines +66 to +67
// Store saves the value in the stash under the key
Store(ctx context.Context, value T)
Copy link
Contributor

Choose a reason for hiding this comment

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

[thought]: Store is a good name. However, intuitively I was expecting it to be Stash. That's in the spirit of Stringer, Writer, Reader etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, the first draft used Stash here. I changed it because it seemed to push too much emphasis on stashing while retrieving is just as (or more) important. Stringer, Writer and Reader have a directed purpose, a Stasher is about a round trip.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me @scothis! ty

reconcilers/stash.go Outdated Show resolved Hide resolved

func TestStasher(t *testing.T) {
ctx := WithStash(context.Background())
stasher := NewStasher[string]("my-key")
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]: I think testing against a stashed value of type string is sufficient. Then again maybe the test becomes more interesting when using a non-primitive type like Example. In particular I am thinking about RetrieveOrEmpty.

Copy link
Contributor Author

@scothis scothis Sep 20, 2024

Choose a reason for hiding this comment

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

All types in go have a default empty value. There's a use of a vanilla struct here:

https://github.com/reconcilerio/runtime/pull/546/files#diff-21cd712a5bbfcb003ce654296346a4008831cce30760e9b6d2d1fb61a0bb5f4dR235

Copy link
Contributor

Choose a reason for hiding this comment

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

I get it. ty

Co-authored-by: Max Brauer <mamachanko@users.noreply.github.com>
Signed-off-by: Scott Andrews <scott@andrews.me>
@scothis scothis merged commit 4a934e2 into reconcilerio:main Sep 20, 2024
4 checks passed
@scothis scothis deleted the stasher branch September 20, 2024 10:37
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