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

Assembly attribute fix for issue #130 - Added DisableFakeItEasyExtensionDirectoryScanAttribute #251

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@megakid

megakid commented Jan 29, 2014

I've tried to enable the ability to disable the directory scanning extensibility logic without impacting current behaviour. Please take a look and see what you think.

James Connor
Added DisableFakeItEasyExtensionDirectoryScanAttribute assembly attri…
…bute to disable static configuration and directory scan for FakeItEasy extensions e.g. IFakeConfigurer etc
@chillitom

This comment has been minimized.

Show comment
Hide comment
@chillitom

chillitom Jan 29, 2014

Nice, however, might I suggest you parameterize the search assembly path [assembly:FakeItEasyExtensionsAssemblies("*.dll")] and use null-empty string to disable the feature [assembly:FakeItEasyExtensionsAssemblies("")] or something like that.

chillitom commented on 92e92a0 Jan 29, 2014

Nice, however, might I suggest you parameterize the search assembly path [assembly:FakeItEasyExtensionsAssemblies("*.dll")] and use null-empty string to disable the feature [assembly:FakeItEasyExtensionsAssemblies("")] or something like that.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jan 30, 2014

Member

Hi, @megakid.

Thanks for the contribution. It's an interesting approach, and I appreciate the simplicity of the solution. It certainly looks like an easy way to disable all the assembly scanning. Unfortunately, I think it's not quite ready for merging as it is, at least right now. Here are my concerns:

  1. Any PR take should come on a branch, not master. Please read the Making Changes section of our CONTRIBUTING.md file. In fact, if you haven't already, it may be worthwhile scanning the whole file.
  2. I'm a little concerned about the lack of tests as well, but these kinds of tests are very difficult to write, so there may not be much to do there anyhow.
  3. There was talk in #130 about implementing a more generic bootstrapper-override system. If we want to do that, I think that's the way to restrict the assembly scanning, and I would like the @FakeItEasy/owners to try to come to a quick consensus to see if that's the approach we want to take. If not, then I could support some refinements to your current approach.

Meet me in the "diff" section for some specific comments related to individual lines of code.

Oh. There was only the one, and it was quite minor.
At first I thought you were stopping scanning of FakeItEasy.dll itself, and I was concerned, but now I see you're not. And I'm not sure we even need to scan it, since the only two extensions defined in there are the default object and string formatters, and we already add them to our formatter cache in ArgumentValueFormatter.ArgumentValueFormatter. Interesting.

Member

blairconrad commented Jan 30, 2014

Hi, @megakid.

Thanks for the contribution. It's an interesting approach, and I appreciate the simplicity of the solution. It certainly looks like an easy way to disable all the assembly scanning. Unfortunately, I think it's not quite ready for merging as it is, at least right now. Here are my concerns:

  1. Any PR take should come on a branch, not master. Please read the Making Changes section of our CONTRIBUTING.md file. In fact, if you haven't already, it may be worthwhile scanning the whole file.
  2. I'm a little concerned about the lack of tests as well, but these kinds of tests are very difficult to write, so there may not be much to do there anyhow.
  3. There was talk in #130 about implementing a more generic bootstrapper-override system. If we want to do that, I think that's the way to restrict the assembly scanning, and I would like the @FakeItEasy/owners to try to come to a quick consensus to see if that's the approach we want to take. If not, then I could support some refinements to your current approach.

Meet me in the "diff" section for some specific comments related to individual lines of code.

Oh. There was only the one, and it was quite minor.
At first I thought you were stopping scanning of FakeItEasy.dll itself, and I was concerned, but now I see you're not. And I'm not sure we even need to scan it, since the only two extensions defined in there are the default object and string formatters, and we already add them to our formatter cache in ArgumentValueFormatter.ArgumentValueFormatter. Interesting.

var appDomainAssemblies = AppDomain.CurrentDomain.GetAssemblies();
var appDomainAssembliesReferencingFakeItEasy = appDomainAssemblies.Where(ReferencesFakeItEasy);
Assembly[] appDomainAssemblies = AppDomain.CurrentDomain.GetAssemblies();
IEnumerable<Assembly> appDomainAssembliesReferencingFakeItEasy = appDomainAssemblies.Where(ReferencesFakeItEasy);

This comment has been minimized.

@blairconrad

blairconrad Jan 30, 2014

Member

Is there a technical reason to change the vars here?

@blairconrad

blairconrad Jan 30, 2014

Member

Is there a technical reason to change the vars here?

/// If you don't know of these interfaces, it's probably fine to disable the directory scan without ill effects.
/// </summary>
[AttributeUsage(AttributeTargets.Assembly)]
public class DisableFakeItEasyExtensionDirectoryScanAttribute : Attribute

This comment has been minimized.

@adamralph

adamralph Jan 30, 2014

Contributor

I think I'd be happier with the name DisableApplicationDirectoryScan.

@blairconrad any opinion?

@adamralph

adamralph Jan 30, 2014

Contributor

I think I'd be happier with the name DisableApplicationDirectoryScan.

@blairconrad any opinion?

This comment has been minimized.

@blairconrad

blairconrad Jan 30, 2014

Member

As it is, it keeps us from scanning the AppDomain as well as the working directory. The only thing we continue scanning is FakeItEasy itself, which

  1. hardly counts, and
  2. I think maybe could be done away with (I was updating my original comment as you were adding this one, so take another peek if you don't know what I'm talking about).

So I'd almost prefer DisableExtensionScan, if we are agreed that this is the right approach. I'm still a little worried about making a change and later wishing we'd implemented a more flexible Bootstrapper. Not that those two systems couldn't live together, but why not have one if we can?

@blairconrad

blairconrad Jan 30, 2014

Member

As it is, it keeps us from scanning the AppDomain as well as the working directory. The only thing we continue scanning is FakeItEasy itself, which

  1. hardly counts, and
  2. I think maybe could be done away with (I was updating my original comment as you were adding this one, so take another peek if you don't know what I'm talking about).

So I'd almost prefer DisableExtensionScan, if we are agreed that this is the right approach. I'm still a little worried about making a change and later wishing we'd implemented a more flexible Bootstrapper. Not that those two systems couldn't live together, but why not have one if we can?

This comment has been minimized.

@blairconrad

blairconrad Jan 30, 2014

Member

Of coures, that name doesn't make a lot of sense if we parameterize as @chillitom suggests.

@blairconrad

blairconrad Jan 30, 2014

Member

Of coures, that name doesn't make a lot of sense if we parameterize as @chillitom suggests.

This comment has been minimized.

@adamralph

adamralph Jan 30, 2014

Contributor

How does the attribute stop AppDomain scanning? It looks to me like it just adds a condition to adding Directory.GetFiles(Environment.CurrentDirectory, "*.dll") to the list of assemblies to scan.

@adamralph

adamralph Jan 30, 2014

Contributor

How does the attribute stop AppDomain scanning? It looks to me like it just adds a condition to adding Directory.GetFiles(Environment.CurrentDirectory, "*.dll") to the list of assemblies to scan.

This comment has been minimized.

@blairconrad

blairconrad Jan 30, 2014

Member

😊 You are, of course, correct.

@blairconrad

blairconrad Jan 30, 2014

Member

😊 You are, of course, correct.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jan 30, 2014

Member

@chillitom, I like the idea of adding some flexibility here. Opt-in is not a bad idea. Of course, we'd want something that would work both with assemblies already loaded in the AppDomain and files that are in the current directory, I think.

Member

blairconrad commented Jan 30, 2014

@chillitom, I like the idea of adding some flexibility here. Opt-in is not a bad idea. Of course, we'd want something that would work both with assemblies already loaded in the AppDomain and files that are in the current directory, I think.

@chillitom

This comment has been minimized.

Show comment
Hide comment
@chillitom

chillitom Jan 30, 2014

Contributor

How about something like the following:

[AttributeUsage(AttributeTargets.Assembly)]
public class FakeItEasyScanPathAttribute : Attribute
{
    internal static string ScanPath { get; private set; }

    static FakeItEasyScanPathAttribute
    {
        ScanPath = "*.*";
    }

    FakeItEasyScanPathAttribute(string scanPath)
    {
         ScanPath = scanPath;
    }
}

If you attach this to your test assembly then it will be initialized before FakeItEasy loads and would set the scan path. If you don't apply it then the static constructor runs when the ScanPath is first accessed by ApplicationDirectoryAssembliesTypeCatalogue and falls back to the current behavior. Optionally you could gate the set in the constructor so only the first assembly to feature such an attribute takes precedence or you could extend the approach to combine the search paths.

(and as before setting a null or empty scan path would signify disabling the feature)

Contributor

chillitom commented Jan 30, 2014

How about something like the following:

[AttributeUsage(AttributeTargets.Assembly)]
public class FakeItEasyScanPathAttribute : Attribute
{
    internal static string ScanPath { get; private set; }

    static FakeItEasyScanPathAttribute
    {
        ScanPath = "*.*";
    }

    FakeItEasyScanPathAttribute(string scanPath)
    {
         ScanPath = scanPath;
    }
}

If you attach this to your test assembly then it will be initialized before FakeItEasy loads and would set the scan path. If you don't apply it then the static constructor runs when the ScanPath is first accessed by ApplicationDirectoryAssembliesTypeCatalogue and falls back to the current behavior. Optionally you could gate the set in the constructor so only the first assembly to feature such an attribute takes precedence or you could extend the approach to combine the search paths.

(and as before setting a null or empty scan path would signify disabling the feature)

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jan 30, 2014

Contributor

Overall I'm favouring the Nancy style bootstrapper approach. If that would take a while to implement, I guess this could serve as simple on/off override in the meantime and could even live on once the bootstrapper is in place, but I feel it would probably warrant deprecation at that time. Currently my feeling is that we should look into the bootstrapper approach before rushing into this.

Contributor

adamralph commented Jan 30, 2014

Overall I'm favouring the Nancy style bootstrapper approach. If that would take a while to implement, I guess this could serve as simple on/off override in the meantime and could even live on once the bootstrapper is in place, but I feel it would probably warrant deprecation at that time. Currently my feeling is that we should look into the bootstrapper approach before rushing into this.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jan 30, 2014

Member

I don't want to take fun work from @megakid , but I'd be content to work on the bootstrapper right away. After all, I've been thinking about it for 7 months… the only hard/slow part would be integration tests, but I don't think any harder than writing tests for the attribute…

Member

blairconrad commented Jan 30, 2014

I don't want to take fun work from @megakid , but I'd be content to work on the bootstrapper right away. After all, I've been thinking about it for 7 months… the only hard/slow part would be integration tests, but I don't think any harder than writing tests for the attribute…

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Jan 30, 2014

Member

@chillitom, I think your sample attribute change is a winner, and would support adopting its approach if we end up going for an attribute-based approach.

Member

blairconrad commented Jan 30, 2014

@chillitom, I think your sample attribute change is a winner, and would support adopting its approach if we end up going for an attribute-based approach.

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Jan 30, 2014

Contributor

@blairconrad I think we should leave this PR open for now and see how you get on with the bootstrapper.

Contributor

adamralph commented Jan 30, 2014

@blairconrad I think we should leave this PR open for now and see how you get on with the bootstrapper.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Mar 3, 2014

Member

@chillitom, @adamralph, given that #130 has been closed, how do we feel about closing this PR?
It was good work, but seems like we're not going to take it.

Member

blairconrad commented Mar 3, 2014

@chillitom, @adamralph, given that #130 has been closed, how do we feel about closing this PR?
It was good work, but seems like we're not going to take it.

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Mar 3, 2014

Member

Oops. Forgot to include @megakid. 😊 Any objection to closing this?

Member

blairconrad commented Mar 3, 2014

Oops. Forgot to include @megakid. 😊 Any objection to closing this?

@adamralph

This comment has been minimized.

Show comment
Hide comment
@adamralph

adamralph Mar 3, 2014

Contributor

Agreed. The issue has been addressed by other means (the bootstrapper).

Contributor

adamralph commented Mar 3, 2014

Agreed. The issue has been addressed by other means (the bootstrapper).

@blairconrad

This comment has been minimized.

Show comment
Hide comment
@blairconrad

blairconrad Mar 4, 2014

Member

Closing, as we have another implementation.

Member

blairconrad commented Mar 4, 2014

Closing, as we have another implementation.

@blairconrad blairconrad closed this Mar 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment