-
Notifications
You must be signed in to change notification settings - Fork 467
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
Can we move AttributesToAvoidReplicating
into DynamicProxy's main namespace?
#517
Comments
I think this class got used a bit more years back when DP didn't have a framework attribute built-in and the Copying from a 2018 comment of mine:
There will still be some use out there for things like Using PGO means that it will then be up to mocking libraries to expose their own API to exclude attributes which is a cleaner API and the way we've gone with a few other APIs over the last few years. |
That made me curious. Is anyone using First, none of the projects that we list on our introductory DynamicProxy documentation page appear to be using
Most projects that bring up a search result across all of GitHub appear to derive from a few repositories; most projects add The only other attributes sometimes added are (like you mentioned:) WCF I have absolutely no problem keeping |
Castle has always been pretty flexible and way too open with the public API surface, and even though we are removing heaps of the public API, I think since this is a pretty trivial amount of code that we should keep the feature rather than remove but mostly because this is going to be used more by people's private projects rather than library code. |
Perhaps true. 👍 I've been prototyping It occurred to me that perhaps partial class ProxyGenerationOptions
{
public IAttributeReplicationHook AttributeHook { get; set; } =
StandardAttributeReplicationHook.Instance;
}
public interface IAttributeReplicationHook
{
bool ShouldReplicateAttribute(Type attributeType, MemberInfo fromMember, Type proxiedType);
}
public class StandardAttributeReplicationHook : IAttributeReplicationHook
{
public static readonly StandardAttributeReplicationHook Instance =
new StandardAttributeReplicationHook();
protected AttributeReplicationHook() { }
public virtual bool ShouldReproduceAttribute(...)
{
return attributeType.IsSubclassOf(typeof(SecurityAttribute)) == false
&& attributeType != typeof(ComImportAttribute)
&& attributeType != typeof(MarshalAsAttribute)
&& attributeType != typeof(TypeIdentifierAttribute);
}
} Pros:
Cons:
This is just an idea, and I'm perfectly happy to keep the present API mostly unchanged (except for the move to PGO)—we don't need to change everything around in v5. I thought I'd mention it anyway in case you think it's worth following up on. |
I was actually thinking similar, I looked at how we could add it to It won't be immutable because the hook object can be mutable or even non-deterministic. I don't think Not really sure which way to go TBH. |
We have some time to ponder this, I found that the main difficulty lies in making the proxy generation Re: breaking change, extending I tend towards adding a new property to PGO, but I'm open to anything really. |
@jonorossi, I'll start with the TL;DR: I suggest doing a PR that changes the namespace of the existing Below I'll try to summarise my thoughts that led to the above suggestion.
This looked like the ideal solution to me. Unfortunately, replacing a static class Since there is already some passing-around of other entities going on, I considered piggybacking on that by bundling all the things being frequently passed around (e.g. options, hook, logger, attributes to avoid replicaing, etc.) into a some Such a context object could be made globally available via a My preliminary conclusion is that changing anything fundamental about |
It doesn't appear it has been a problem in the past with this static class applying to the whole runtime, however a unit test project (or even a mocking library) could add an attribute which changes what the SUT actually does because this is static and the mocking libraries no longer ilmerge DP into their assembly. I'd still like to see us remove this static. I had a look through the usage of Do you still have the code where you attempted to add a new collection property to PGO? I'd much prefer we went this way. I'd hardcode the 4 runtime attributes as we already do for |
@jonorossi, I didn't keep my original code but I recreated a version similar to it. See the PR referenced above this post. |
Shall we close this issue? It's probably not worth holding back the next release just because of this. |
AttributesToAvoidReplicating
currently sits in the sub-namespaceCastle.DynamicProxy.Generators
. It's one of only two public types left in that namespace—the namespace contains mostly internals that became trulyinternal
in #505.My aim here is to concentrate DynamicProxy's public API in just one single namespace:
Castle.DynamicProxy
.I therefore suggest that we do either of two things:
AttributesToAvoidReplicating
into the main namespaceCastle.DynamicProxy
; orProxyGenerationOptions
I haven't thought about (2) too deeply, but (1) would be simple enough to do. Also, it's likely not a part of the public API that one uses a lot, so perhaps it's not worth investing too much time.
Any thoughts or opinions?
The text was updated successfully, but these errors were encountered: