Skip to content
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

Create Internal Interfaces? #12

Closed
Haukinger opened this issue Aug 23, 2018 · 9 comments
Closed

Create Internal Interfaces? #12

Haukinger opened this issue Aug 23, 2018 · 9 comments

Comments

@Haukinger
Copy link

This works fine with internal classes, but when I try to use an internal interface as settings container, I get Type 'SimpleConfig.Dynamic.InterfaceImplementations._MyAssembly.IServiceSettings_Impl' from assembly 'SimpleConfig.Dynamic.InterfaceImplementations, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' is attempting to implement an inaccessible interface.

I've tried to make my InternalsVisibleTo "SimpleConfig", but that doesn't help. Any idea?

@Haukinger
Copy link
Author

Figured it out: internals have to be visible to SimpleConfig.Dynamic.InterfaceImplementations

But the dynamic creation of implementations does not seem to apply to properties within the interface, i.e. those remain null, even if decorated with CustomEnumerable.

@Haukinger
Copy link
Author

Sorted this one out, too: GetCustomAttributes(true) does not returned attributes declared in or on implemented interfaces, so PropertyHelper.GetMappingStrategies needs to manually look through all implemented interfaces:

To do that, I added .Concat( @this.DeclaringType.GetInterfaces().SelectMany( x => x.GetProperties().Where( y => y.Name == @this.Name ).SelectMany( y => y.GetMappingStrategies() ) ) ) between Where and ToArray at the beginning of the method.

@spadger
Copy link
Owner

spadger commented Dec 31, 2018

Good to see you got around this. TBH though, I'd be inclined to use POCOs instead of interfaces - the interface stuff was really a bad excuse to play with codegen.

POCOs are easier to reason with and control IMHO, and easier to test without mocking config.

@Haukinger
Copy link
Author

I prefer the interfaces, because they work without setters on the properties. With pocos, technically anybody can modify the settings.

@spadger
Copy link
Owner

spadger commented Dec 31, 2018

I believe your Poco could be

public class SomeConfig
{
    public string SomeValue { get; private set }
}

And obviously it's a valid defensive position to make, but conversely (it took me a number of years to reach this point), do you rate your colleagues so low that they'd be modifying a config instance after initialisation? Possible, but not probable

@Haukinger
Copy link
Author

It works with a private setter, just not for read-only properties. You might add the line to GetMappingStrategies just for completeness' sake, in case someone tries an interface property in an interface.

As for the colleagues... they'll do anything you can or cannot imaging, so I prefer to be safe than sorry :-D

@spadger
Copy link
Owner

spadger commented Jan 1, 2019 via email

@spadger
Copy link
Owner

spadger commented Feb 22, 2019

Hi,

I no longer have access to a windows machine, and getting this stuff up and running in netstandard so I can develop and test it is not trivial (e.g. TypeGenerator.cs(132, 51): [CS7069] Reference to type 'Assembly' claims it is defined in 'System.Runtime', but it could not be found after adding back the missing runtime and code-generation packages)

I'm not sure I have the energy to fix this on a mac

@spadger spadger closed this as completed Feb 22, 2019
@spadger
Copy link
Owner

spadger commented Feb 22, 2019

For completeness, I took a look at this code with Mono, and I'm hesitant to add your addition. The current code is for a specific property instance that happens to have a name. Your code will find mapping strategies for any property that has the same name, but a class can have multiple properties with the same name if they're an explicit interface implementation, so the mapping strategies could end up being a superset of the legal strategies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants