Add input type-checking to Commands #190
Conversation
shartse
force-pushed
the
pretty-print
branch
from
70209e7
to
0062baf
Compare
sdb/command.py
Outdated
| @@ -240,6 +240,29 @@ def _call(self, | |||
| """ | |||
| raise NotImplementedError() | |||
|
|
|||
| def __input_type_check( | |||
| self, objs: Iterable[drgn.Object]) -> Iterable[drgn.Object]: | |||
There was a problem hiding this comment.
I think that this also needs to accept any walkable type, at least for Locator's. see Locator.caller(). How much benefit do Locator's get from this new checking? If not much, then I think only non-Locator, non-PrettyPrinter commands would be getting new checking (of which there are not very many). Note that Walker also has this checking built in, but that could be removed in favor of the Command type-checking.
There was a problem hiding this comment.
Could we add some examples in the test suite that make sure that Walkers and Locators can still be passed things like void * and still work after this change?
e.g.
sdb> addr spa_namespace_avl | cast void * | avl | spa
ADDR NAME
------------------------------------------------------------
0xffff944eccfd4000 rpool
Codecov Report
@@ Coverage Diff @@
## master #190 +/- ##
==========================================
+ Coverage 85.36% 85.62% +0.26%
==========================================
Files 56 56
Lines 2309 2324 +15
==========================================
+ Hits 1971 1990 +19
+ Misses 338 334 -4
Continue to review full report at Codecov.
|
| cur_type != prev_type): | ||
| raise CommandError( | ||
| self.name, | ||
| f'expected input of type {self.input_type}, but received ' |
There was a problem hiding this comment.
Seems like this should be printing valid_input_types, rather than self.input_type
| @@ -281,15 +304,19 @@ def call(self, objs: Iterable[drgn.Object]) -> Iterable[drgn.Object]: | |||
| # the command is running. | |||
| # | |||
| try: | |||
| result = self._call(objs) | |||
| if self.input_type and objs: | |||
There was a problem hiding this comment.
Are subclasses allowed to have input_type=None but _valid_input_types() is not the empty set? I think this should be the case for Walk() and PrettyPrint().
|
I think that |
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: #159
What is the new behavior?
Refactored the input-type checking to be a generator and added it to the generic
Commandimplementation.Note - because we actually support multiple input and output types for certain commands (
spaandvdev), I added extra logic to check forInputHandlerfunctionality. This might not be the cleanest solution - maybe exact command should have aSetfor input and output types instead of a single string? Maybe we want to revisit the idea of multiple input/output types at all?Does this introduce a breaking change?
Other information