Change ExecuteUpdate to accept Action instead of Func parameter#35455
Change ExecuteUpdate to accept Action instead of Func parameter#35455roji merged 2 commits intodotnet:mainfrom
Conversation
roji
force-pushed
the
MoreExecuteUpdate
branch
from
cab9383 to
691c082
Compare
| _ => Visit(node), | ||
| }; | ||
|
|
||
| // TODO: Insert necessary Convert nodes etc. when the expected and actual types differ |
There was a problem hiding this comment.
Unless you have strong feelings about this, I'd rather not systematically pollute our repo with every TODO that I put in code... In many cases that's useful, but in cases like this I really don't see the value of it, and would rather not do it just because we have a rule... Let me know - I'd ultimately rather remove the TODO, but I don't think that's a positive thing.
There was a problem hiding this comment.
We can discuss this again, but generally, non-tracked TODO comments make the code look less professional and usually don't have enough context to properly reason about for someone else. I'd much rather have an issue with a checklist of all the small changes in an area so that they can be discussed and prioritized
There was a problem hiding this comment.
OK, sure thing. I guess I don't generally consider TODO comments as looking unprofessional (regardless of whether they reference an issue or not), and I'm just looking to reduce process and needless issues. CSharpToLinqTranslator specifically is an extreme case as there really are a lot of unimplemented cases (many theoretical, some less so).
But I'm open to doing as you say here, maybe e.g. a single issue for all pending CSharpToLinqTranslator isn't too bad.
2 participants
This is a small follow-up to #35257, so part of #32018 (/cc @AndriySvyryd).
This changes ExecuteUpdateAsync to accept an Action instead of a Func, following @aradalvand's suggestion here. This improves the developer experience further, removing the need to have a
returnat the end of the provided lambda. This required some non-trivial changes to CSharpToLinqTranslator, since ExecuteUpdateAsync now expects anAction<UpdateSettersBuilder>parameter, but the translated lambda type was currently determined by the body, and so wasFunc<UpdateSettersBuilder, UpdateSettersBuilder>.This also renames the oddly-named SetPropertyCalls to UpdateSettersBuilder, which better expresses what that type really is (it really is just a builder, which accumulates setter lambda pairs). This is technically an additional breaking change, but any scenario in which SetPropertyCalls was explicitly referenced is already broken by #35257 - so this rename shouldn't in itself affect anyone.