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
feat(context): adding ctx as first argument #136
base: master
Are you sure you want to change the base?
Conversation
@philippgille if you like the direction, I will continue for the rest of the packages... |
29a80ec
to
d8e9ca7
Compare
Thanks for the draft, and in general for all your recent contributions! 💪 So this PR is one valid approach, and also one that other popular libraries have used in the past, when they moved from functions without context, towards functions with it. For example the Redis library v6 only had The approach I originally had in mind was the rough one: Just change the signatures and willingly break backward compatibility. To make it a bit smoother, I was thinking of releasing a version that has updates for all stores to use the latest minor and patch versions of their dependencies, and update to latest supported Go versions. #108 and #109 were meant for that, but I wanted to have a more solid testing first before a release. I managed to switch from Travis CI to GitHub Actions in #110 and use Mage as build tool in #111, but one TODO I didn't get to was testing on Windows, as well as some other minor tasks.
That way no one feels forced to migrate to the newest one, as they still get all the latest dependencies in the former, and they have time to migrate the context passing. Benefit on our side is that the But maybe there's another option, something in between: While checking the Redis changelogs between v6 and v9 for your other PR, I noticed that Redis introduced a I think I'm currently still leaning towards the hard cut, after a fresh release without context. But totally open to hear more opinions around this. |
@philippgille I'm fine with both hard-cut and mine I really dislike the I suggested this PR as I'm not aware of your timeline and release schedule when you want to do the hard cut... another option is to leave the anyways, thanks for the comments, as long as we making progress, I'm happy ))) |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 56.69% 62.04% +5.35%
==========================================
Files 11 25 +14
Lines 822 2274 +1452
==========================================
+ Hits 466 1411 +945
- Misses 327 752 +425
- Partials 29 111 +82 ☔ View full report in Codecov by Sentry. |
done, see #139 |
Huh that's an interesting idea! I'll think about it! |
@philippgille what are the next steps ? should I re-base or you taking a different approach ? |
Sorry for the delay, replied in #107 (reply in thread) |
To maintain backward compatibility addint WithContext set of functions instead of changing the signatures of existing ones. The Ctx parameter is not used yet in many functions yet. The usage will come in the following PR after interface is changed. Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@philippgille congrats on the new release! what is the decision on |
Yes, let's move forward as outlined in the discussion. Regarding the implementation itself, I like the approach of making the Thanks for working on this! 💪 |
Great! @philippgille |
|
Thank you for calrifications!
Correct. Both old and new methods are covered by same tests.
Agree completely. Once this is merged, I will add context checking everywhere.
Yep, I can do that after this is merged. |
Something else came to mind: With some store configs having a timeout parameter, we should either document that they are completely ignored, or in the |
I can add timeout to all store configs if you want, I was doing this for some already, and can expand to the rest... |
@philippgille let’s continue on this one ? |
To maintain backward compatibility
addint WithContext set of functions
instead of changing the signatures of existing ones.
The Ctx parameter is not used yet in many functions yet.
The usage will come in the following PR after interface is changed.
Signed-off-by: Boris Glimcher Boris.Glimcher@emc.com
mentioned in #100 (comment)