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
DynComms [0/n]: fn
package additions
#8653
base: master
Are you sure you want to change the base?
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
464abe1
to
22b714d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I wonder where you get the inspiration for these library functions, these functions are almost suitable for a general golang library using generics ?
@@ -136,3 +138,48 @@ func TestZipWith(t *testing.T) { | |||
z, []bool{false, true, false, false, false}, | |||
)) | |||
} | |||
|
|||
func TestPropForEachConcMapIsomorphism(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the TestProp
prefix makes sense to be generalized in our docs, as soon as we do prop testing ? So we have a consistent way throughout the codebase ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree. Our prop testing is pretty sparse at the moment but it's my preferred form of testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sounds good, will also create property tests next time, they seem to include also the fuzzing component by creating a whole range of input values.
Maybe we can come up with a nice doc guide in the near future what we expect in general from unit tests in the LND codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah what's also nice about them is that it forces you to think more clearly about what the precise requirements of the system are, as opposed to eyeballing a few test cases and hoping that those cases are enough to make reasonable implementations likely. It's a brittle assumption.
foundIdx := FindIdx(pred, s) | ||
|
||
// onlyVal :: Option[T2[A, B]] -> Option[B] | ||
onlyVal := MapOption(func(t2 T2[int, uint8]) uint8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if it makes sense to generalize it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generalize what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapping a tuple to an Option Tuple to a value: [T2[A, B]] -> Option[T2[A, B]] -> Option[B]
, could be part of the tuple definition ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be. However, I do think that maybe exporting functions for Fst
and Snd
is a good idea.
Years of writing Haskell as my native language. Yeah this would be better suited for a golang general lib but I have no desire to do ongoing maintenance of go libraries for the whole golang ecosystem. Further, @Roasbeef doesn't want to depend on external libraries for this kind of thing due to OSS maintainer risk (either malice or just disappearing), which is why we invented the Generally speaking, the |
|
||
func TestPropConstructorEliminatorDuality(t *testing.T) { | ||
f := func(i int, s string, isRight bool) bool { | ||
Len := func(s string) int { return len(s) } // smh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: what does the comment mean smh ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Shake my head", means disappointment. I was making some commentary about how I couldn't just put len
in the field as the type func(string) int
even though it would be a sensible thing to be able to do. append
has similar issues. Perhaps I should either remove the snarky comment or be more explicit about warning people not to try and refactor it away.
@@ -136,3 +138,48 @@ func TestZipWith(t *testing.T) { | |||
z, []bool{false, true, false, false, false}, | |||
)) | |||
} | |||
|
|||
func TestPropForEachConcMapIsomorphism(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sounds good, will also create property tests next time, they seem to include also the fuzzing component by creating a whole range of input values.
Maybe we can come up with a nice doc guide in the near future what we expect in general from unit tests in the LND codebase.
foundIdx := FindIdx(pred, s) | ||
|
||
// onlyVal :: Option[T2[A, B]] -> Option[B] | ||
onlyVal := MapOption(func(t2 T2[int, uint8]) uint8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapping a tuple to an Option Tuple to a value: [T2[A, B]] -> Option[T2[A, B]] -> Option[B]
, could be part of the tuple definition ?
dff47f3
to
728055a
Compare
@ProofOfKeags, remember to re-request review from reviewers when ready |
NOTE: This change is part of a series implementing Dynamic Commitments.
Change Description
This adds some useful library functions that will allow us to capture patterns we often use throughout the codebase. None of the code in this PR has any direct bearing on Dynamic Commitments but it is generally useful infrastructure that will be assumed to be available in future PRs in the series.
Steps to Test
N/A
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.