Improve python debugger client. #2441
Conversation
zherczeg
force-pushed
the
debugger_client_py
branch
from
95c7941 to
c00a5e6
Compare
jerry-debugger/jerry_client.py
Outdated
| result = self.debugger.set_break(args) | ||
| if self.debugger.not_empty(result): | ||
| print(result.get_data()) | ||
| sys.stdout.write(self.debugger.set_break(args)) |
There was a problem hiding this comment.
what's the reason for rewriting lots of prints to sys.stdout.write? print is a lot more pythonic, sys is too low-level. unless there is a good reason, it's generally better practice to use print. (even for printing to stderr, if that's needed somewhere.)
There was a problem hiding this comment.
+1 for keeping the print
There was a problem hiding this comment.
Same problem as putstr, adds an unwanted newline. People recommended to use stdout.write.
There was a problem hiding this comment.
I'm not completely sure what the unwanted newline may be. This code is printing to the console and users typically expect info to be broken into lines. If this is just part of the output, it may be a signal that this function should not print anything but return a string, perhaps, that can be composed by the caller into a full line of text and printed there.
Anyway, if the only feature that's missing is the no-newline-at-the-end-of-printing, then consult the manual: https://docs.python.org/2/library/functions.html#print . (print('text to be printed', end='').
There was a problem hiding this comment.
Strings returned by the debugger client contains newlines, including terminating newlines. If you print them with print you get two newlines, and that is ugly. The problem with end='' that it is python 3. The current one is compatible with any kind of python.
I have a suggestion: I can create a utility, e.g yield function:
def yield(str):
sys.stdout.write(str)
And use yield everywhere. What do you think?
There was a problem hiding this comment.
This function would be in the main program, not in the Debugger module, which should print nothing at all (and no need to know anything about where the output goes). No need for self in this utility as well.
There was a problem hiding this comment.
I don't understand what this means. I see no Debugger python module, and 'main program' (if it means the main function) is in this file where we are discussing the the pros and cons of print vs utility functions.
If 'main program' stands for the whole jerry_client.py file, then I've already commented on that approach (see: module-level global function used by a single class only -- ugly and awkward).
There was a problem hiding this comment.
The debugger client has two files at the moment: jerry_client_ws.py which is the Debugger Client module, and jerry_client.py application which uses the module to provide the user interface. The user interface is not something reusable, just a simple application. Adding utility functions there is normal.
There was a problem hiding this comment.
Just because something can be technically done doesn't mean that it is good design. Even the whole class in the jerry_client.py could be dropped and replaced with global functions and some global state. But that also doesn't mean that it is the way to go.
There was a problem hiding this comment.
The jerry_client.py is not a single class file. It contains a class because python command line support designed this way, but that class is just a utility class. It cannot be avoided unless we use another command line system. So this is not a design, it is just forced on us.
The core function is the main() function, which is a message loop processor.
Patch updated.
zherczeg
force-pushed
the
debugger_client_py
branch
2 times, most recently
from
f3ab7bf to
a37959b
Compare
|
Any more comments? |
zherczeg
force-pushed
the
debugger_client_py
branch
from
a37959b to
d77b51b
Compare
jerry-debugger/jerry_client.py
Outdated
| prompt.cmdloop() | ||
|
|
||
| if prompt.debugger.not_empty(args.exception): | ||
| if args.exception != None: |
There was a problem hiding this comment.
In python, this should be written as args.exception is not None
jerry-debugger/jerry_client_ws.py
Outdated
| self._exec_command(JERRY_DEBUGGER_MEMSTATS) | ||
|
|
||
| def set_break(self, args): | ||
| if args == "": |
There was a problem hiding this comment.
This could also be if not args as at the beginning of def delete.
jerry-debugger/jerry_client_ws.py
Outdated
| JERRY_DEBUGGER_FUNCTION_NAME_END]: | ||
| _parse_source(self, data) | ||
| result = _parse_source(self, data) | ||
| if result != "": |
There was a problem hiding this comment.
This could also simply be if result:
jerry-debugger/jerry_client_ws.py
Outdated
| if _set_breakpoint(debugger, command, True): | ||
| set_result = _set_breakpoint(debugger, command, True) | ||
|
|
||
| if set_result != "": |
jerry-debugger/jerry_client_ws.py
Outdated
| if _set_breakpoint(debugger, command, True): | ||
| set_result = _set_breakpoint(debugger, command, True) | ||
|
|
||
| if set_result != "": |
jerry-debugger/jerry_client_ws.py
Outdated
| result += _enable_breakpoint(debugger, function.lines[function.first_breakpoint_line]) | ||
|
|
||
| if not found and not pending: | ||
| if result == "" and not pending: |
There was a problem hiding this comment.
if not result and not pending:
akosthekiss
added
tools
zherczeg
force-pushed
the
debugger_client_py
branch
from
d77b51b to
76a2c5f
Compare
|
Thank you for the review. Patch updated. |
|
Why did you change the file permissions of |
|
It is a library, should never be executed directly. |
zherczeg
force-pushed
the
debugger_client_py
branch
from
76a2c5f to
b576c9e
Compare
Replace DisplayData to DebuggerAction. The new class has only four type options. Furthermore several functions returning with DisplayData is changed to return with string. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com
zherczeg
force-pushed
the
debugger_client_py
branch
from
b576c9e to
79bd1c1
Compare
3 participants
Replace DisplayData to DebuggerAction. The new class has only four type options. Furthermore several functions returning with DisplayData is changed to return with string.