Bottom answer of https://stackoverflow.com/questions/43688450/how-to-obtain-argparse-subparsers-from-a-parent-parser-to-inspect-defaults had some great pointers, works in both 2 and 3.

Should we switch this to None and then fill in the correct default value below? Re: the default-parameter-gets-re-used-in-Python feature.

I think the () above is fine because () are immutable.

Should we do result.custom_plugin_paths = data.get('custom_plugin_paths', [])?

Great catch, I caught this too after reviewing it again, reminded me of bloodhound code.

What was the rationale for os.walk vs os.listdir? Seems like with listdir we could reduce the code complexity here.

Nevermind, I see that it was already like that. You can fix it if you want though :)

Most definitely :)

Is Directories containing ... clearer? Reading the code, it seems like things will break if we specify a file here.

Sure, that makes sense, ++

Noting that it now works for directories and files.

Should these be lists so that it matches the actual interface?

Great catch! It was originally a tuple, so I must have missed a spot 👍

I think if we renamed this decorator to def tuplify (or whatever the canonical verb is) it would be a lot easier to understand. Alternatively, I'm not opposed to just putting it in the function body.

I generally prefer genericized naming too, but as written the argument has to be named custom_plugin_paths_function, otherwise it won't work, so I'm ambivalent. Maybe I can try to make it so the decorator tuplifies all arguments 🤔 , ++.

If we put it in the function body, I don't think we could lru_cache it, right?

I'm not sure if I understand why the parameter has to be named custom_plugin_paths_function. Can't we s/custom_plugin_paths_function/fn/g?

I don't know where the lru_cache is applied right now, so we can leave it as a decorator if it works as intended already.

*Ouch my bad @OiCMudkips, I mis-typed last Friday, the argument to the function being decorated has to be named custom_plugin_paths, as it's currently written. So it's not applicable to any function or any function with 1 arg, only 1 arg func's with custom_plugin_paths as an arg.

I will try to make it so that it can accept any 1 arg func, so we can name it a more genericized name though.

re: where the lru_cache is applied
I wrap functions that accept a list, to turn them into functions that accept a tuple, before the lru_cache decorator gets to them, so that they can be cached. Lists are unacceptable for immutability reasons IIRC

Following up with this: the reason the arg must be named custom_plugin_paths, is because any function decorated by this decorator may be called with custom_plugin_paths as a keyword argument.

i.e. If I changed this to

    def wrapper_of_custom_plugin_paths_function(one_arg):
        return custom_plugin_paths_function(tuple(one_arg))

it would result in

============================================== ERRORS ===============================================
________________________________ ERROR collecting tests/main_test.py ________________________________
tests/main_test.py:76: in <module>
    class TestMain:
tests/main_test.py:294: in TestMain
    'name': 'Base64HighEntropyString',
tests/main_test.py:33: in get_list_of_plugins
    for name, plugin in import_plugins(custom_plugin_paths=[]).items():
E   TypeError: wrapper_of_custom_plugin_paths_function() got an unexpected keyword argument 'custom_plugin_paths'

I'm not sure if I understand this, can you explain?

If you do --help with --custom-plugins, you want to see the options for e.g. --no-custom-detector, if we just parse --help before --custom-plugins, it will SystemExit and give the help message without any options from the custom plugins.

Due to the issue explained in this comment, super calls in Python 2 for plugins no longer work!

e.g. the super call in

    @property
    def __dict__(self):
        output = super(HighEntropyStringsPlugin, self).__dict__
        output.update({
            'hex_limit': self.entropy_limit,
        })

        return output

will throw a super(type, obj): obj must be an instance or subtype of type error because it thinks self is not a subtype, when really it just has different addresses for its functions. I patched around the same issue with issubclass in this highlighted code, but with super there is not a desirable path forward that I can see, I shall ask on StackOverflow for advice.

For posterity, I did look into this, although I time-boxed myself.

You can change the metaclass of BasePlugin to be e.g.

+class AlwaysTrue(ABCMeta):
+    def __instancecheck__(self, instance):
+    def __subclasscheck__(self, instance):

however, super still throws the above error, only isinstance and issubclass work.

In other words, I believe this is definitely blocked on dropping Python 2 support #260. Either that or have the user supply the import path as well as the file path, but I don't want to do that.

Had to do

-        assert all(
-            func(plugin.__class__) == func(Base64HighEntropyString)
-            for func in (str, dir)
-        )
+        assert str(plugin.__class__) == str(Base64HighEntropyString)
+        assert dir(plugin.__class__) == dir(Base64HighEntropyString)

because otherwise it was 94% 21->exit, 71->exit

The % go into this format string

Great catch!! I no longer need the [:4] so that makes a lot of sense 🎊

This seems kinda excessive, can we just call tuple(argument) wherever we want to try to be safer?

One alternative is to make it so that the ArgumentParser returns a tuple by implementing a custom action but honestly I'd be OK with us just not worry about the accidentally mutating the list of paths.

The reason for tuplifying is so that we can lru_cache the functions this decorator is used on, not b/c I'm afraid the list will get mutated.

I'll re-read the code to see if tuplifying as soon as we parse args (/the baseline) makes things easier. 👍 Convert to X as early as possible seems like always good advice, I'll have to see if there was a reason I wrote it this way. 🤔

It does not appear there was a good reason 😁 I'll go ahead and do the custom action, something similar to https://stackoverflow.com/questions/50365835/how-to-make-argparse-argumentparser-return-a-tuple-or-an-np-array-rather-than-a