-
-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix trimming runtime error #491
Fix trimming runtime error #491
Conversation
Thanks for trying to solve the issue. However, I don't think this approach is a change I can accept. Adding the |
I'm sorry. Could you clarify which new()? All the constraints for e.g return Activator.CreateInstance<TModel>(); which is just the strict equivalent of the following at compile time: return new TModel(); I can keep the original Satisfies around if you want, but the internal code would not use it. But for the other (for TApp), it's better to have compile time validation than runtime failure (e.g |
Oh, that's because you have a |
Make CommandLineUtils working with net6.0 trimming. Use explicit constructors known at compile time instead of using Activator.CreateInstance
I pushed a new commit that is reverting most of the changes and fixing only my original issues. Doesn't break the existing API nor it does introduce a new API. |
Yes, thank you. The new update looks much better. And you confirmed this resolves your runtime trimming issue? I haven't played with this feature of .NET 6 before, so don't know if there is a good way we can add tests to ensure this doesn't regress in the future. |
It looks like this causes some unit tests to fail in the CI workflow. Is this working on your computer? |
My apologize, I pushed a fix. |
Oh, one error with formatting, let me fix this |
Codecov Report
@@ Coverage Diff @@
## main #491 +/- ##
==========================================
+ Coverage 82.21% 82.28% +0.07%
==========================================
Files 106 106
Lines 3289 3302 +13
==========================================
+ Hits 2704 2717 +13
Misses 585 585
Continue to review full report at Codecov.
|
Just pushed a commit to try to improve slightly the coverage of |
Thanks @xoofx! This will be in the 4.0.1 patch. |
Cool, thanks a lot, I could remove my workaround! 😉 |
Hey!
I discovered that CommandLineUtils is not trimming compatible with net6.0 and will fail at runtime with an error like:
I have been able to workaround it here but it's not super great.
Which is really unfortunate because we usually want to use trimming for small console apps. 😅
This PR is fixing this issue with these attributes used via
Activator.CreateInstance
and use instead explicit constructors. The previous abstractionGetValidationAttr<T>
was removed.It should make CommandLineUtils a bit more compatible with net6.0 trimming.
I tried to enable trimming analysis by adding net6.0 target... but wow, that triggers quite a few errors. Not a lot, but as it gets viral, it might require more deeper thinking on this.
At least, this PR fixes a basic usage of this library.