Monitor subcommand: interactivity improvements #272
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I agree that it's a good idea to replace |
|
I've actually not used cantools myself in quite a while and I can't recall how the monitor user experience was. I trust that your change is good for the majority of the users. I will not try it myself. |
letting this parameter default to True makes the test much easier to debug because things are printed where something goes wrong instead of at the end and the formatting of the expected and the actual results is identical to enable easy comparison using a diff tool. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
in contrast to an iteratable, we do not need to know how often the method is called using this approach and what the result of a given call is. In some sense, we now actually test something... Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
parsing large files takes a few seconds, so for the user it is nice to know that something is going on. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
this avoids messing up the filter string with stuff like KEY_LEFT, KEY_RIGHT, etc. In the medium term, it might be a good idea to use something like the readline library for the input of the filter string. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
the cursor can be moved, and characters can be deleted in the middle of the filter string using the backspace and delete keys. Still, switching this to something more sophisticated like GNU readline would be beneficial IMO. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
… filter regex IMO this is more intuitive than disabling filtering. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
this is quite useful for messages that contain many signals. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
this issue got much more pronounced with the introduction of single-line scrolling. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
This ought to be the same anyway, but at least on my machine, running `setup.py install` does not install an executable called `cantools`. The same might be true for `pip`. Note that this only fixes the instructions for people who use a UNIX-like operating system, on Windows the name of the python interpreter might be different. Thanks to [at]eerimoq for the review comments. Signed-off-by: Andreas Lauser <andreas.lauser@mbition.io> Signed-off-by: Oliver Kopp <oliver.kopp@mbition.io>
729d96c
to
2441a91
Compare
|
ok. fixing the tests was quite elaborate, mainly because I initially had serious issues understanding the error messages. I also did not extend the tests to check for every single new interactive behavior as this would IMO be a bit too excessive. |
eerimoq
reviewed
Feb 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR mainly improves the interactivity of the
monitorsubcommand to make it feel much more "less-like" and to make editing the filter regular expression much more convenient. Besides this, the last patch changes the instructions inREADME.rstto usepython3 -m cantoolsinstead ofcantoolson the CLI, because the latter does not always seem to work. Please yell at me if you want to have that patch reverted.Andreas Lauser andreas.lauser@mbition.io, Mercedes-Benz AG on behalf of MBition GmbH.
Imprint