-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fn: generalize type of t in UnwrapOrFail #9122
base: master
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
As stated in #9141 review, this is a NACK from me. If we do this we should tightly define the testing interface we want to conform to and then add a separate file that defines it and its expected properties, rather than putting it in option.go or other specific type files. |
Return the stored value, not zero.
This is needed to have require.EventuallyWithT function.
It is needed to pass other types satisfying needed interfaces to methods Option.UnwrapOrFail and Result.UnwrapOrFail. An example of such a type is *lntest.HarnessTest. It embeds *testing.T and has all the methods but can't be passed directly as *testing.T, but can be passed now as an instance of Testing interface. Also *testing.B and assert.CollectT (from "require" package). Methods Helper() and FailNow() are optional and they are called if exist. Also added the test for UnwrapOrFail methods of Option and Result.
I moved the interface to a separate file (testing.go) and separated it to multiple parts. The base interface (TestingT) has only method I added a test for the method testing it with real |
I found a bug in |
Is this still true, I remember finding the same bug and fixed it, I thought. EDIT: Yeah I fixed this here |
Change Description
It is needed to pass other types satisfying needed interfaces to methods
Option.UnwrapOrFail
andResult.UnwrapOrFail
.An example of such a type is
*lntest.HarnessTest
. It embeds*testing.T
and has all the methods but can't be passed directly as*testing.T
, but can be passed now as an instance ofTesting
interface. Also*testing.B
and assert.CollectT (from "require" package).Methods Helper() and FailNow() are optional and they are called if exist.
Also added the test for UnwrapOrFail methods of Option and Result.
Also fixed a bug in
Result.UnwrapOrFail
. It used to return zero value in case of success. It should return the value stored.Steps to Test
This is an internal change needed to simplify development.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.