Open kiddinn opened 3 years ago
I would like to propose major simplification of the current magic framework. Please find some details below.
class MagicProtocol(typing.Protocol):
def __call__(
data: Text,
*args:Union[Text, int, bool],
**kwargs:Union[Text, int, bool]
) -> Any:
pass
Argument types would be defined separately for each type. For example argument of type int would be defined as:
class MagicIntArgument:
flags: List[Text]
default: Optional[Int]
help: Text
def add_to_argparse(self, parser argparse.ArgumentParser):
# code to transform MagicIntArgument to argparse.ArgumentParser.add_argument invocation
...
Similar classes would be defined for other argument types (Text, bool and others as needed).
picatrix_magic
decorator signature would stays the same:
def picatrix_magic(
function: Optional[MagicProtocol] = None,
arguments: Optional[List[MagicArgument]] = None,
name_func: Optional[Callable[[Text], Text]] = None,
conditional: Optional[Callable[[None], bool]] = None
) -> MagicProtocol:
...
If arguments
is provided, it will be used to parse magic arguments and provide them to decorated function. \
If it's not provided, arguments will get parsed out of decorated function definition
def _magic_to_arguments(magic: MagicProcotol) -> List[MagicArgument]:
hints = typing.get_typing_hints(magic)
...
Rest of the code will stay more or less the same, but will follow the path in which arguments
is provided - either explicitly by caller or parsed out of magic definition.
@bladyjoker, @kiddinn - I would like to ask your opinion on following topics:
Any
type is a broad one and kind of defeats a purpose of having a type at all. Initially I was thinking about making it pd.DataFrame
, but then I saw that Timesketch magics are using Dict and other types as output. Do you see any way to define all possible valid outputs of the Magic?typing.Generic
. Do you happen to have an experience with Python generics and can share your opinion here? Alternatively - can you suggest any environment where I can quickly check my assumptions about typing
? My usual development cycle is to validate assumptions in REPL and then code the most succesful solution. Unfortunately type verification is disabled in runtime.functools.partial
s (reformatting docstrings to take into the account that some attributes got already applied by partial). Do you have any objections against this dependency?ok, to answer your questions here
Return type. It is hard to predict the future need of a return type, often times it is a DataFrame, but it can be a dict, it can be a single value, int, string, etc... it can also be none... as in timesketch_set_active_sketch
which doesn't produce an output. You may want to write a magic that returns a bool to indicate whether it was successful or not, that's the reason for teh Any
, I agree it is not ideal, and it would be good to have something else, but I'm not sure what we can do here without restricting the magics to a limited set/usability.
We need to start adding more typing verification... we can use tools like mypy
to automatically run against the codebase (I've been experimenting with it, but I need to make some changes to have it work in the first place). This could run on every PR, just like pylint and unit tests, to make sure the codebase is enforcing typing. My idea is also to get rid of the docstring parsing to enforce
the typing and use the build-int typing to do the casting and enforcing of arguments.
I don't have any objections about that.
I was also thinking, do we need to have the magic arguments at all, or just have the typing in the functions and parsing of docstrings? Do we need the added flexibility but still a lot more complexity by also supporting defining these MagicArguments?
ATM when you initialize a magic class it has an optional argument of
arguments
, which is a list of MagicArgument.If it is not supplied it is ignored and arguments are derived from a docstring.
This could be detangled, and MagicArgument be better defined and used throughout. Also it should not be done in the
__init__
function but arguments should be configured after initialization of the object.