Deprecate ModuleScope.DefineType

[stakx]

This is in response to #399 (comment):

[@stakx:] [...] deprecating ModuleScope.DefineType should be fine.

[@jonorossi:] I'm definitely in favour of it, I don't like that others are emitting into the DP assembly [...].

The single known reason for using ModuleScope.DefineType was to allow mocking libraries to create delegate proxies, the enhancements from #436 now provide a cleaner way to do that, so we can go ahead and deprecate ModuleScope.DefineType.

[stakx]

@jonorossi, this might be a good occasion to try out the public API change approval. Happy to wait for your PR (#440) to go in first, then rebase this one.

[jonorossi]

@stakx good idea, I'll get #440 sorted.

[jonorossi]

I've merged #440, you can rebase this pull request now.

[stakx]

@jonorossi, seems to work as it should:

Errors, Failures and Warnings

1) Failed : Castle.PublicApiTestCase.PublicApi
  ref/Castle.Core-net35.cs does not match Castle.Core.dll
  Expected string length 303613 but was 303712. Strings differ at index 195077.
  Expected: "...dModuleName { get; }\r\n        public System.Reflection.Emi..."
  But was:  "...dModuleName { get; }\r\n        [System.ObsoleteAttribute("E..."
  ----------------------------------------------^
at Castle.PublicApiTestCase.PublicApi()

Nice! 👍 I'll run this locally later today to see the diff tool pop up & update the public API.

[jonorossi]

Nice! 👍 I'll run this locally later today to see the diff tool pop up & update the public API.

No diff tool pops up, I think that was a feature of the now obsoleted part of the library, the "approver" part. I wouldn't have wanted that anyway, however you'll see the diff with your git client.

[stakx]

@jonorossi, thanks for the correction, I see now. This is working beautifully.

[jonorossi]

I expected to see EditorBrowsableAttribute in the API output, however the tool is explicitly written to exclude that attribute: https://github.com/JakeGinnivan/ApiApprover/blob/80f1fd97db4499d7929cc582a5b7f4eb7f85c018/src/PublicApiGenerator/ApiGenerator.cs#L407

It doesn't really worry me since it is trivial but was surprising.

[stakx]

@jonorossi - Would you prefer if we deprecate DelegateProxyGenerator (which you suggested here) at the same time (i.e. as part of this PR), or shall we deal with that in a separate PR?

On second thought, let me retract that question. A separate PR for DelegateProxyGenerator (and possibly more) seems cleaner to me, no need to mix up two different things even though both are related to delegate proxies.

[jonorossi]

@stakx I agree, a separate PR done for another release sounds like the way to go.