chore(codenav): Resolve repo and commit in common code#63072
Conversation
kritzcreek
left a comment
There was a problem hiding this comment.
Very, very nice. Thank you for doing this.
|
Caution License checking failed, please read: how to deal with third parties licensing. |
bca3c8c to
ae487bc
Compare
|
Caution License checking failed, please read: how to deal with third parties licensing. |
There was a problem hiding this comment.
This missing initialization before setting a field later was caught by rapid. 🎉
There was a problem hiding this comment.
Not exactly sure what this is, I'll try to minimize it later.
There was a problem hiding this comment.
@kritzcreek I thought we should have a basic test for this generator, not sure how you feel about this.
There was a problem hiding this comment.
It don't mind the test as a usage example (it's cute), but it doesn't increase my confidence in the Bool implementation much.
There was a problem hiding this comment.
I removed this and added a Pearson's Chi-Squared test for WithProbabilities instead.
There was a problem hiding this comment.
I know this might seem a bit overkill, but I figured since we need the recursive traversal anyway, adding a cache is not that much extra work.
There was a problem hiding this comment.
Yeah, adding the cache seems fine even if we can't produce a filter with more than one repository reference for now. Finally I can filter my usages for (not (not (not (not "github.com/sourcegraph/sourcegraph"))))
kritzcreek
left a comment
There was a problem hiding this comment.
Code looks good in general. Added a few smaller nits, but nothing that should block you from merging once you've fixed the entropy crash
There was a problem hiding this comment.
WDYT about shadowing the args variable here so we don't accidentally use the unresolved version further down.
There was a problem hiding this comment.
The scoping in Go doesn't match Rust/ML, so that would be an error since the type would change. It would require introducing another function (or a block). However, I can change the variable names so that this is args and the other one is unresolvedArgs, so the latter becomes more inconvenient to type.
There was a problem hiding this comment.
It don't mind the test as a usage example (it's cute), but it doesn't increase my confidence in the Bool implementation much.
There was a problem hiding this comment.
The correct way to check the sign of a float would be to math.Copysign(1, p.Chance) < 0
There was a problem hiding this comment.
I'm guessing this is for the -0.0 case? I think it doesn't matter if it is -0.0 here?
There was a problem hiding this comment.
Yes, probably doesn't matter here. Just thought it might be an interesting bit of trivia
There was a problem hiding this comment.
Yeah, adding the cache seems fine even if we can't produce a filter with more than one repository reference for now. Finally I can filter my usages for (not (not (not (not "github.com/sourcegraph/sourcegraph"))))
391cd37 to
199f207
Compare
This PR replaces https://github.com/sourcegraph/sourcegraph/pull/63057. We introduce
a new type which carries information about the repo, the deserialized cursor etc.
Test plan
Added some property based tests to check that the function doesn't panic
on different kinds of inputs.
Changelog