-
Notifications
You must be signed in to change notification settings - Fork 158
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
C# support (suggestion) #208
Conversation
Great work @matthid 👍 |
Thanks! Yeah I already used it in several projects before I did the pull request. It all works fine when used with a modified FSharp.Compiler.Service.dll. |
Good work! Is this already fixed in FCS? Where is the pull request?
|
There is none, because I still don't know what I have actually done with my changes of if they are a good idea generally :) |
Ok please send it anyway and mark it as WIP. Don and others can help and
|
Is there any way to just get in the simple path fix without waiting for the Compiler services? just change line 151 of the Main.fs in Metadataformat to this: let nice = name.Replace(".", "-").Replace("`", "-").Replace("<", "").Replace(">", "") And the crashing goes away. We can then continue to work on improving the c# docs |
@Vidarls please send PR. will merge and release asap. |
Note that the fist commit does exactly that and it even filters all invalid characters so this error can't happen again. But note that the rest of the first commit is not suited for a merge... You should add && (typ.IsClass || typ.IsInterface) at line 668. |
I prefer a separate pull request. Since CI and stuff... |
Ok? |
Note also that there is actually one reason to merge all commits immediately: This way if someone wants to use C# support he only has to bundle the fixed FCS.dll instead of bundling FSharp.Formatting as well. @Vidarls are you working on it or should I split it? |
I have a pull req almost ready |
I also encountered an issue when trying to generate documentation for a C# dll that contained events: Version:
|
That should be recorded as an FCS issue (and use a try/with as a workaround for now) |
@dsyme sorry for the late reply. Where & and how should I report this issue for the Compiler Services? |
….Compiler.Service commit, c5dfd4dd488f6dcd1024b0ed2b564ce9d2d414fa).
866b3cf
to
e8a6fc3
Compare
e8a6fc3
to
3b113f7
Compare
Note: I updated this branch to exclude everything that has been done and merged by @Vidarls. Now it can be merged again. |
Thanks for the update! I merged this - the only change I did is that I manually incorporated the change from @ovatsus in #223 where he also filters out members that have the Are you using this for some specific C# project? It would be great to have a test for this - I guess we could add a sample C# project to the tests, or we could just include a DLL+XML of a project that you're using and check that it works for that one. |
I think I saw a flag somewhere to include documentation for private members? In this case one would assume that those are included, but it will probably not matter that much. Yes I used this for several projects.. A public one is RazorEngine. However until FCS (fsharp/fsharp-compiler-docs#229) is fixed you do not get that much documentation to test :). The xml file is already included in the nuget package so .dll and .xml files are already here. |
I've taken a look at this. You should not really be calling ReturnParameter on a non-standard C# Event at all - this gets the type of an event if it is considereed to be like an IEvent-typed property by the F# type system. But some events are "non-standard" and can't be given a good type as a property, and these must be called using That said, I'm adjusting FCS to still give a ReturnParameter with the delegate type of the event in this case. I'm also adding properties EventIsStandard, EventAddMethod and EventRemoveMethod to allow you to be more fine-grained if you need to be. |
This is a suggestion on how to add C# support.
While now C# assemblies will now be processed we still have the following problems:
The other commits add support for Assembly information in a type (you can add something like: Defined in Assembly "Other.dll" to you type template), see https://github.com/matthid/RazorEngine/blob/develop/doc/templates/reference/type.cshtml#L32. Now the namespaces are sorted (when you have multiple assemblies).