ScanBy: Update docs
#882
ScanBy: Update docs
#882
Conversation
Codecov Report
@@ Coverage Diff @@
## master #882 +/- ##
==========================================
- Coverage 92.53% 92.38% -0.15%
==========================================
Files 108 110 +2
Lines 3428 3441 +13
Branches 1020 1020
==========================================
+ Hits 3172 3179 +7
- Misses 194 200 +6
Partials 62 62
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Unfortunately, this PR makes two unrelated changes. Can we please have each in their own PR?
Also, if ScanBy doesn't need changes with respect to #803 then let's enable nullable context on its unit tests, which is a good way to exercise its signature.
reduce captures size
I think it reduces an allocation, but not the capture size? Either way, it would be best done as a sweeping change in one PR?
Sure. |
| KeyValuePair.Create("bar" , 1), | ||
| KeyValuePair.Create((string)null, 3), | ||
| KeyValuePair.Create("foo" , 1)); | ||
| KeyValuePair.Create<string?, int>("foo", 0), |
There was a problem hiding this comment.
Type explicit to improve nullability behavior since KeyValuePair.Create() enforces a nullability value without target-typing to the sequence nullability type.
There was a problem hiding this comment.
You could go with typing just the one needed:
result.AssertSequenceEqual(
KeyValuePair.Create((string?)"foo", 0),
KeyValuePair.Create((string?)null , 0),
KeyValuePair.Create((string?)"bar", 0),
KeyValuePair.Create((string?)"baz", 0),
KeyValuePair.Create((string?)null , 1),
KeyValuePair.Create((string?)null , 2),
KeyValuePair.Create((string?)"baz", 1),
KeyValuePair.Create((string?)"bar", 1),
KeyValuePair.Create((string?)null , 3),
KeyValuePair.Create((string?)"foo", 1));There was a problem hiding this comment.
I considered that, but I honestly like the other one better. Generally, to me, an explicit type cast like this feels like I may have done something wrong to need the cast, whereas target-typing using the explicit type of .Create() feels more natural. Just my opinion though.
viceroypenguin
changed the title
ScanBy: Update docs and reduce captures sizeScanBy: Update docs
|
@viceroypenguin Any follow-up? |
|
I'm not sure what your concern is. Per
the "reduces capture size" change was reverted. This PR was also updated per your request to include
There is an unanswered expressed opinion here, but otherwise the only thing necessary for this PR is to resolve conflicts with latest. |
No concern. Just this:
Title of this PR reads says it updates the documentation (unrelated to #803), but it does more. |
|
If you resolve conflicts with mainline, the additional changes will work themselves out. I'll let you take care of that at your leisure. |
This is your contribution. If you don't wish to or lost the motivation to see it through, including resolving conflicts and undoing unrelated changes, then that's fine. I'll go ahead and close it for now but happy to re-open if you'd like to help. |
This PR adds to #803. It updates documentation.
ScanBydoes not need other changes to address nullability documentation.