Conversation
GeertvanHorrik
requested review from
SimonCropp,
ltrzesniewski and
tom-englert
| bool runPeVerify = true, | ||
| Action<ModuleDefinition>? afterExecuteCallback = null, | ||
| Action<ModuleDefinition>? beforeExecuteCallback = null, | ||
| string? folderName = null, |
There was a problem hiding this comment.
If you move this new parameter to the end, it should be fully backwards compatible
There was a problem hiding this comment.
I have though about it. You mean behind the assemblyName or really at the end? On the long term, I think it's better to keep them together, but short-term would be best to add at the very end.
There was a problem hiding this comment.
I agree the folderName parameter should be put at the very end, this avoids a breaking change.
ltrzesniewski
left a comment
ltrzesniewski
left a comment
There was a problem hiding this comment.
I had a very similar issue in InlineIL, and I had to basically rewrite WeaverTestHelper, so I completely understand the need for this change. 🙂
| bool runPeVerify = true, | ||
| Action<ModuleDefinition>? afterExecuteCallback = null, | ||
| Action<ModuleDefinition>? beforeExecuteCallback = null, | ||
| string? folderName = null, |
There was a problem hiding this comment.
I agree the folderName parameter should be put at the very end, this avoids a breaking change.
|
Updated the PR based on feedback. Thanks! |
3 participants
See #1297