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

Add ConfigurationDependencyAttribute to help with referenced assemblies ... #898

Conversation

augustoproiete
Copy link
Member

… not being copied over.

As per discussion on #880

@codecov-io
Copy link

codecov-io commented Nov 9, 2016

Current coverage is 73.68% (diff: 100%)

Merging #898 into dev will increase coverage by 0.04%

@@                dev       #898   diff @@
==========================================
  Files            69         70     +1   
  Lines          3195       3200     +5   
  Methods         695        697     +2   
  Messages          0          0          
  Branches        358        358          
==========================================
+ Hits           2353       2358     +5   
  Misses          672        672          
  Partials        170        170          

Powered by Codecov. Last update 9dd1be8...5c5f5e2

@@ -0,0 +1,27 @@
using System;

namespace Serilog.Build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think Serilog.Settings.ConfigurationDependencyAttribute is a more self-describing name and namespace for the type. The attribute is only likely to be used in conjunction with XML or JSON settings, so focusing it on that scenario has some discoverability benefits even if it could be used for other purposes.

/// Gets the dependent type reference.
/// </summary>
/// <value>The dependent type reference.</value>
public Type DependentType { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependent, to me, is the thing that "does-the-depending" (i.e. the inverse direction). Maybe Dependency?

/// <summary>
/// Initializes a new instance of the <see cref="ImplicitDependencyAttribute"/> class.
/// </summary>
/// <param name="dependentType"></param>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A type from the assembly that is used dynamically.

@augustoproiete
Copy link
Member Author

@nblumhardt PR amended based on your comments

@nblumhardt
Copy link
Member

Looks good, thanks @caioproiete.

I wondered for a moment if DependencyType might be an instance of Hungarian notation, but on consideration, the Type may not be the actual Dependency. Any thoughts on this?

@augustoproiete
Copy link
Member Author

I wondered for a moment if DependencyType might be an instance of Hungarian notation, but on consideration, the Type may not be the actual Dependency. Any thoughts on this?

No strong feelings there. The Type suffix seems commonly used for property exposing a Type (1, 2, 3, 4, ...).

But yes, the Type is not really the dependency... The dependency is the assembly and in an ideal world, we would like to do something like [ConfigurationDependencyAttribute(typeof(SinkName).Assembly)] but, of course, that doesn't work on attributes.

@nblumhardt
Copy link
Member

👍 let's stick with DependencyType.

Need to investigate #880 a little further before merging, but LGTM here.

@augustoproiete augustoproiete changed the title Add ImplicitDependencyAttribute to help with referenced assemblies ... Add ConfigurationDependencyAttribute to help with referenced assemblies ... Nov 9, 2016
@augustoproiete
Copy link
Member Author

Bug fixed with most recent version of ReSharper.

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

Successfully merging this pull request may close these issues.

None yet

3 participants