-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Defer part rejection exception in VSTypeScriptFormattingService to time of construction #58940
Conversation
...ore/Portable/ExternalAccess/VSTypeScript/Api/IVSTypeScriptFormattingServiceImplementation.cs
Outdated
Show resolved
Hide resolved
src/CodeStyle/Core/CodeFixes/Host/Mef/CodeStyleHostLanguageServices.cs.cs
Show resolved
Hide resolved
@jasonmalinowski Based on the comments from @zkat , I plan to keep this pull request in its current form and marking it as auto-merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's switch to the AllowDefault pattern since we already have other PRs in flight expecting that.
...ore/Portable/ExternalAccess/VSTypeScript/Api/IVSTypeScriptFormattingServiceImplementation.cs
Outdated
Show resolved
Hide resolved
…quire it in the constructor This change defers the MEF part creation exception in MEF 2 scenarios from the call to GetExports to the call (if any) to Lazy<>.Value. Rather than reject the part from the catalog entirely, this change allows the part to be included as long as it is not accessed.
public VSTypeScriptFormattingService([Import(AllowDefault = true)] IVSTypeScriptFormattingServiceImplementation impl) | ||
=> _impl = impl ?? throw new ArgumentNullException(nameof(impl)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably deserves a comment here explaining why we are doing AllowDefault but then still blocking it. Feel free to add as a follow-up since CI is green.
https://sharplab.io seems to fail even after this merge, see.: ashmind/SharpLab#922 |
@bernd5 That issue was filed before this change was merged. Do you have information about the failure which still occurs? |
@sharwell: Oh, I have mixed up a commit. |
Fixes #58817 (comment)