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

"class-instead-of-function" anti-pattern in Executor class #18

Closed
mgeier opened this issue Feb 10, 2020 · 1 comment
Closed

"class-instead-of-function" anti-pattern in Executor class #18

mgeier opened this issue Feb 10, 2020 · 1 comment

Comments

@mgeier
Copy link
Contributor

mgeier commented Feb 10, 2020

I've already mentioned this in #12 (comment), but I think this deserves its own issue.

During the move from nbconvert to nbclient, the Executor class evolved into something that's not really a meaningful class anymore. Instead, it is something that takes a few input parameters and produces an output. In other words, it turned into a function disguised as a class.

This is clearly visible by its usage here:

return Executor(nb=nb, resources=resources, km=km, **kwargs).execute()

This was somewhat better in nbconvert (see https://github.com/jupyter/nbconvert/blob/8c9bffc8deb65ced99b20684ddd148fd885ad9e8/nbconvert/preprocessors/execute.py#L765-L766):

    ep = ExecutePreprocessor(**kwargs)
    return ep.preprocess(nb, resources, km=km)[0]

I think there are two possible ways to improve this, I don't know which one is better in this case:

  1. Get rid of the Executor class and implement everything in the stand-alone execute() function. This would of course become a huge function, so probably some parts of it should be factored out in separate (probably internal) functions and classes.

  2. Make the Executor great again. An Executor class should be instantiated with some set of options and it should provide an execute() method that can be repeatedly (and ideally concurrently) be used to execute multiple notebooks with those given options.

@MSeal
Copy link
Contributor

MSeal commented Mar 31, 2020

Closing this for now, see comment in PR

@MSeal MSeal closed this as completed Mar 31, 2020
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

No branches or pull requests

2 participants