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

ENH: make OutputChecker pluggable #141

Merged
merged 2 commits into from
Mar 20, 2024
Merged

ENH: make OutputChecker pluggable #141

merged 2 commits into from
Mar 20, 2024

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Mar 2, 2024

Add CheckerKlass attribute to DTConfig for the class name of the Checker, use in instantiating the Runner. Then it percolates all the way up to all sorts of frontends, both doctest and pytest layers.

DTConfig gets a bit crowded, and it becomes easy to silently ignore some keys: VanillaOutputChecker ignores atol/rtol etc. If this ever becomes a problem, can add more structure to DTConfig.
The right thing is likely to make DTConfig hold actual objects not classes and their instantiation parameters.
That would be a larger change though, so let's get there when it gets more usage.
Until then, let's keep it simple.

cross-ref gh-38.

@ev-br ev-br added the enhancement New features w.r.t. the original refguide-check label Mar 2, 2024
@ev-br
Copy link
Member Author

ev-br commented Mar 5, 2024

cc @Sheila-nk

@ev-br
Copy link
Member Author

ev-br commented Mar 5, 2024

Note to self: with this and alternate DTParsers, the docs should shift the focus a bit. First discuss that everything is pluggable (Checker, Parser, Finder), then get on to the fp-aware Checker.

Now that I think of it, the Finder is a bit different: the main logic is in find_doctests, so maybe it should be split into a separate Finder? Not sure.

@@ -340,7 +343,7 @@ def __init__(self, checker=None, verbose=None, optionflags=None, config=None):
if config is None:
config = DTConfig()
if checker is None:
checker = DTChecker(config)
checker = config.CheckerKlass(config)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea. However, the implementation currently does not make the Checker pluggable since doctest.OutputChecker has no __init__ function and can therefore not take any config argument like DTChecker.
Any attempt to use the Vanilla OutputChecker will fail:

    checker = config.CheckerKlass(config)
E   TypeError: OutputChecker() takes no arguments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is OutputChecker here, is it doctest.OutputChecker? Then indeed, it'll fail. ISTM the best we can do is to document the story: "Any admissible OutputChecker must have __init__(self, config)".

@Sheila-nk
Copy link
Collaborator

The right thing is likely to make DTConfig hold actual objects not classes and their instantiation parameters.

Maybe before we get to this, we can override the vanilla OutputChecker to have an __init__function like you did during testing. What do you think?

@ev-br
Copy link
Member Author

ev-br commented Mar 12, 2024

Maybe before we get to this, we can override the vanilla OutputChecker to have an __init__function like you did during testing. What do you think?

Not sure what OutputChecker you mean?
All in all I wouldn't rush changing the way DTConfig is organized. WHat's there now seems to work, and enhancements very well might be YAGNI.

@ev-br ev-br merged commit 2af7ffa into main Mar 20, 2024
4 checks passed
@ev-br ev-br deleted the alt_checker branch March 20, 2024 08:12
@ev-br
Copy link
Member Author

ev-br commented Mar 20, 2024

Merged it, thanks for the review Sheila!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features w.r.t. the original refguide-check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants