auto Install of Revivers only triggered if no reviver exists

[workabyte]

I would like to create a reusable library for custom revivers of common data types that will be automatically installed by Args when included in my project.

currently the SearchAssemblyForRevivers method is called when

• manual call to Args.SearchAssemblyForRevivers
• Can Revive can not find a reviver

if my custom reviver is of a typed that already has a parse method such as bool/int/DateTime then the only way to get that reviver registered is manually. think that explains what me and @Geraint87 were experiencing in case #77.

Also because I am looking to have my revivers packaged into an internal nuget package that will be available ans reusing on all of our cmdline projects these revivers are in a separate assembly. Ideally I would be able to register these revivers with little effort on the users end so they do not need to take any action.

[workabyte]

I am already working on a solution to this, do you have any special direction you would like me to take?
Initially I have added

        private Args()
        {
            SearchAssemblyForRevivers(); //calling search on construct so revivers are installed at creation instead of can revive not seeing a reviver
        }

then I modified SearchAssemblyForrevivers as follow

            var searchAllAssemblies = ConfigurationManager.AppSettings["PowerArgs-SearchAllAssemblies"];
            if (!string.IsNullOrWhiteSpace(searchAllAssemblies) && searchAllAssemblies.ToLower().Equals("true"))
            {
                foreach (var assembly in AppDomain.CurrentDomain.GetAssemblies())
                {
                    ArgRevivers.SearchAssemblyForRevivers(assembly);
                }
            }
            else
            {
                a = a ?? Assembly.GetCallingAssembly();
                ArgRevivers.SearchAssemblyForRevivers(a);
            }

we can take a few different approaches to this if you have an alternate preference just let me know

[workabyte]

honestly I would prefer to invert the option and always install from all assemblies at ctor.
figured there was a reason that you were forcing a specific assembly though.

[adamabdelhamed]

I like what you've done. Thanks for the work. Let me take a closer look at some point over the weekend before pulling it in.

There was no special reason for picking a particular assembly. I must admit I didn't realize you could do AppDomain.CurrentDomain.GetAssemblies(). That's pretty cool.

[adamabdelhamed]

I would recommend a small change to your tests (which are failing). I think you need to add a try / finally where you set your config setting to false in the finally.

Your test reviver always returns a date of Today even if the input is bad. This is causing an older test to fail that makes sure a bad date input throws an exception.

[workabyte]

im going to modify a little bit and then will setup a new pull

[workabyte]

side note i am getting some hits on Re-Sharper for some small clean up. would you want me do some refactoring and add some JetBrains Annotations?

[workabyte]

just realized im breaking one edge case test because of the test date time reviver i added.
fixing that up now, will commit shortly

[workabyte]

@adamabdelhamed, do i need to do anything further for this to get pulled in?

thanks again for the great lib

[adamabdelhamed]

Very sorry for the delay, but after thinking about it i do have a little bit more feedback. I don't like that we're using configuration files to set options. PowerArgs has tons of options, and they're almost all configurable via attibutes that modify the underlying program definition. This feature should work the same way. Rather than config files, I would prefer this approach.

1. Add a property to CommandLineArgumentsDefinition called something like SearchAllAssembliesForRevivers. I think it should have a default value of false.2. Create a class that derives from ArgHook, maybe called SearchAssembliesForReviversAttribute or something like that. Override the BeforeParse method and in that override set the SearchAllAssembliesForRevivers property to true. This file should live in the HelperTypesPublic folder since it will be public.3. Somewhere in the Args class, probably in ParseInternal, before any revivers are called, check the SearchAllAssembliesForRevivers and if it's true then enable your new code that searches assemblies.

I'm sorry for being so picky, but if we do it this way then the option lives on the definition like anything else and can be enabled via attributes like anything else.
Does that make sense?
Thanks,Adam
Date: Mon, 3 Oct 2016 12:37:02 -0700
From: notifications@github.com
To: PowerArgs@noreply.github.com
CC: adamabdelhamed@live.com; mention@noreply.github.com
Subject: Re: [adamabdelhamed/PowerArgs] auto Install of Revivers only triggered if no reviver exists (#100)

@adamabdelhamed, do i need to do anything further for this to get pulled in?

thanks again for the great lib

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[workabyte]

makes sense, possible we could take the following approach

1. make it a class level attribute
2. default to true and allow for the attribute to set to false eg: [DoNotSearchAllAssemblies]

I think having the default as true for this option may be more inline with the expectations of how the application would handle finding revivers and in the event this is a pain point for someone trying to get max speed out of it then they can turn it off. there is very little additional overhead by searching all by default.

No worries on the review / feedback it's good stuff and makes a lot of sense