-
Notifications
You must be signed in to change notification settings - Fork 572
dotnet-svcutil VB support and add test. #5386
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
Conversation
dce51c8 to
2c15744
Compare
|
I missed the 1-year anniversary for this PR, but only by a few days. We're still hoping for its adoption. |
|
Hi Carol, what's the status with this? Is there still a possibility that it might make it into production? |
|
+1 |
1 similar comment
|
+1 |
|
Could you tell us more about the What does that signal regarding the intent for this PR? |
|
This needs to happen. :) |
|
Hi Kathleen, do you have any visibility into this? |
|
Thank you for your participation in this. It's an important feature. What's next? Is there a way to follow its progress up the pipeline? |
|
@InteXX Sorry, I just tried something, I'm not from Microsoft. |
Oh, OK. |
|
Carol, it'd be nice to hear from you regarding your pull request. Is there a way we can get this to the attention of the people who can make it happen? |
Thank you, I have already done it some days ago. Hi @mconnew
There has been a PR open for two years to generate WCF client code in VB, so the work already appears to be done. Is it possible to merge this PR? |
Aha. Yes, I saw that one. Well stated. I remember being impressed. |
|
|
||
| if (projects.Length == 1) | ||
| if(csProjects.Length == 1 && vbProjects.Length ==0 ) | ||
| { |
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.
nit: code formatting is off.
|
@imcarolwang, I only got pinged on this late last year so wasn't really aware it existed. I've looked through the PR and my only concerns are some code formatting, and can we see if we can use the runtime shipped nuget package System.CodeDom? |
|
Thank you. |
|
@skurth, we're going to get this merged, but it only provides VB support. To do this for F#, there are a few hurdles we would need to clear. The first hurdle is finding out does FSharpCodeProvider have sufficient features to generate a valid client? From the project documentation website it says (emphasis added):
It's possible that some needed CodeDom constructs aren't implemented. Somebody needs to prototype (and this PR is a good basis for doing so) whether even a basic scenario is doable. The second problem is the license of the FSharp.Compiler.CodeDom package (needed for FSharpCodeProvider) is unusual, it's the unlicense. I would need to talk to Microsoft legal to see if that's something we'd even be allowed to depend on. If it's not something we could ship with (we can't just depend on it as this codebase is used for WCF Connected Services too), then a more creative solution might need to be done. One possible solution would be to have a command line parameter(s) to configure an explicit CodeDomProvider implementation, which would require you to download the FSharp CodeDom package and point dotnet-svcutil to the package folder. At that point, we'd probably say it's too much effort and tell you to generate the client into a separate csharp project and reference that project from the FSharp project. |
Hi @mconnew, Thanks for taking a look and for the suggestions. Regarding System.CodeDom: you’re right that it ships the VB provider via Microsoft.VisualBasic.VBCodeProvider. However, in our repo we currently carry a forked CodeDOM stack and providers under src/dotnet-svcutil/lib/src/FrameworkFork/Microsoft.CodeDom/**. That fork includes namespace/type differences from the runtime System.CodeDom implementation, and dotnet-svcutil-lib uses those forked CodeDOM types and CodeDomProvider.CreateProvider(...) discovery broadly (not just in VBCodeProvider.cs, but also across codegen fixups and other framework-fork components, including the C# provider path). Because of this, switching to the runtime System.CodeDom VB provider isn’t a simple file replacement.it would effectively require migrating the surrounding CodeDOM “type universe” (Microsoft.CodeDom.* → System.CodeDom.*) and updating provider resolution accordingly. Doing this as a narrow change risks mixing two incompatible CodeDOM implementations and could introduce regressions or output/formatting differences (VB and potentially C#). I think it makes more sense to consider adopting System.CodeDom as part of a larger effort to remove the forked framework/codegen sources, where we can migrate holistically and delete the duplicated forked CodeDOM implementation in one coordinated change. Separately, I agree the PR has gone a bit stale. It was left in draft earlier due to a change in direction at the time. I will bring it up to date with the latest changes and address the formatting feedback. I’ll get it ready for review so we can move forward with merging. |
I'm absolutely fine with that. I only care about VB, not about the few F# devs ;-) Thank you and @imcarolwang very much! You will make many companies and VB-devs very happy. |
|
@imcarolwang, totally understand and agree for this PR about forking the VB code provider. As a non-urgent follow-up task can you work to switch out the forked CodeDom implementation to the nuget version? It should be relatively standalone without lots of dependencies on other pieces of code so should be doable isolated without removing the entire forked codebase. |
Thanks for approving the PR. Merging it now. I’ll follow up by looking into switching the forked CodeDom implementation to the System.CodeDom NuGet package. |
Linked issue: #4381.