93 change regex command #94
Conversation
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
==========================================
+ Coverage 98.58% 98.62% +0.04%
==========================================
Files 69 69
Lines 2117 2181 +64
==========================================
+ Hits 2087 2151 +64
Misses 30 30
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -12,16 +20,17 @@ class Command(Protocol): | |||
|
|
|||
| #: A flag which indicates whether calling of the method should trigger an interrupt | |||
| interrupt: bool | |||
| # format: Optional[str] = None | |||
There was a problem hiding this comment.
| # format: Optional[str] = None |
|
|
||
| @abstractmethod | ||
| def parse(self, data: bytes) -> Optional[Sequence[AnyStr]]: | ||
| def parse(self, data: AnyStr) -> Optional[Tuple[Sequence[AnyStr], int, int, int]]: |
There was a problem hiding this comment.
Feels like Tuple[Sequence[AnyStr], int, int, int] should be given a NewType declaration or even a dataclass
| message_end, | ||
| ) = parse_result | ||
| if match_end != message_end: | ||
| # We want the whole (formatted) message to match a command |
There was a problem hiding this comment.
Documentation of the whole / complete would be good to have in the class docstring
| # can we use signature instead to more cleverly do conversions? i.e. if | ||
| # type hints aren't available just pass on with no conversion? Otherwise | ||
| # we can get unhelpful error messages. | ||
| # Also, should we deal with str/bytes conversions seperately? | ||
| # Create a mapping function that does conversion rather than argtype(arg)? | ||
| # It would take args and inspect.Parameters as parameters? | ||
| # Is type hints enough? - doesn't know about non-hinted vars |
There was a problem hiding this comment.
This is definitely worth investigating at some point. My current thoughts are:
- The case of no type hints should be accounted for, either by raising an error or dumping the raw
AnyStron the user - Due to the complexity of
bytes<->stringit makes most sense to dump the rawAnyStron the user and let them deal with it, the complexity of passing through encodings / decodings seems unnecessary when the user can do it in their endpoint. - A mapping function would be nice if we could come up with one that satisfied users needs across the board, but I frankly don't see that happening in a way that's better than using the default casts of the types
There was a problem hiding this comment.
I think discussions like this are better had in new issues than in code comments/PRs. Can we remove the code in the comment and make a new issue?
| @@ -36,24 +36,50 @@ def __call__(self, func: Callable) -> Callable: | |||
| setattr(func, "__command__", self) | |||
| return func | |||
|
|
|||
| def parse(self, data: bytes) -> Optional[Sequence[AnyStr]]: | |||
| def parse(self, data: AnyStr) -> Optional[Tuple[Sequence[AnyStr], int, int, int]]: | |||
There was a problem hiding this comment.
This can be drastically simplified by removing all encoding / decoding if we merge #91 first
|
Closing this as not planned. When it becomes desired again raise a new issue, linked to this #177. |
Requires #89
Fixes #92 and #93
Required for #90
RegexCommandto parse from data of typeAnyStrrather than justbytes.RegexCommand.parsenow also returns the start and end indices of the match within the message, as well as the length of the message matched against. This gives enough information forMultiCommandInterprerterto strip off a matched command and forCommandInterpreterto check for a full match.CommandInterpreter.handlelogic updated to use the new return signature.RegexCommandandCommandInterpreter