Row now disposable #1835
Row now disposable #1835
Conversation
TomFinley
requested review from
Ivanidzo4ka,
artidoro,
codemzs,
eerhardt,
sfilipi,
wschin and
yaeldekel
| /// via the <see cref="DisposeCore(bool)"/> functionality. | ||
| /// </summary> | ||
| /// <param name="disposing"></param> | ||
| protected sealed override void Dispose(bool disposing) |
There was a problem hiding this comment.
disposing [](start = 52, length = 9)
just curious, why not call it "dispose"? #Resolved
There was a problem hiding this comment.
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern
I think this is MS recommendation.
In reply to: 239561491 [](ancestors = 239561491)
There was a problem hiding this comment.
Yes, as @Ivanidzo4ka says, disposing is preferred. I only diverged from the pattern sparingly, since disposal/finalization is one of those things that hard enough to get "right" that slavish devotion to the pattern really helps keep the code maintainable and correct, right down to their recommendations to the names of the parameters, the order of the if statements, order of base calls, and so on, and so on.
| public override bool IsColumnActive(int col) | ||
| { | ||
| return _row.IsColumnActive(col); | ||
| return Input.IsColumnActive(col); |
There was a problem hiding this comment.
return [](start = 24, length = 6)
you tend to prefer => for other one-liners.
There was a problem hiding this comment.
Kinda. Sometimes. I agree this seems like a situation where normally I'd prefer the => syntax.
| private abstract class TypedRowBase | ||
| private abstract class TypedRowBase : WrappingRow | ||
| { | ||
| protected readonly IChannel Ch; |
There was a problem hiding this comment.
protected readonly IChannel Ch; [](start = 12, length = 31)
Not related to PR, but why we have channel here?
All it does is just work as IExceptionContext. Why it has to be IChannel?
ghost
locked as resolved and limited conversation to collaborators
3 participants
Fixes #1824 .
Note that some internal things that instead operate on top of delegates will still have
Actiondisposer delegates, but my expectation is that most of those things are (or should be) disposable.The usual advice about the commits being a useful way to review still apply, though less so than in prior PRs since there are fewer bulk renamings than elsewhere.