Splitting the debugger and console part of the python debugger #2406

TamasZakor · 2018-06-20T07:32:17Z

Move DebuggerPrompt to jerry_client.py
Implement JerryDebugger functions in the jerry_client_ws.py file
Server response is displayed by jerry_client.py

JerryScript-DCO-1.0-Signed-off-by: Tamas Zakor ztamas@inf.u-szeged.hu

zherczeg

Good direction.

zherczeg · 2018-06-20T10:05:34Z

jerry-debugger/jerry_client.py

+            return -1
+
+
+def print_source(debugger, line_num, offset):


This could be part of the debugger, retuning a list of lines

zherczeg · 2018-06-20T10:06:12Z

jerry-debugger/jerry_client.py

+
+        if result is None:
+            continue
+        elif '\3' in result:


zherczeg · 2018-06-20T10:06:32Z

jerry-debugger/jerry_client.py

+            break
+
+        prompt.debugger.mainloop()
+        result = prompt.debugger.smessage


Why can't mainloop() return with this?

zherczeg · 2018-06-20T10:07:30Z

jerry-debugger/jerry_client_ws.py

+        self._exec_command(JERRY_DEBUGGER_FINISH)
+
+    def next(self):
+        """ Next breakpoint in the same context """


Are these comments are used?

robertsipka · 2018-06-21T05:09:00Z

jerry-debugger/jerry_client.py

+        self.quit = False
+        self.cont = True
+        self.debugger.non_interactive = False
+        self.dsp = 0


What this variable means or serve? This should be a more talkative name.

robertsipka · 2018-06-21T05:21:09Z

jerry-debugger/jerry_client.py

+            if line_num >= 0:
+                print(self.debugger.print_source(line_num, 0))
+            elif line_num == 0:
+                print(self.debugger.print_source(self.debugger.default_viewrange, 0))


This will never be reached (the previous condition already checked it).

robertsipka · 2018-06-21T05:28:25Z

jerry-debugger/jerry_client.py

+    debugger.src_offset_diff = int(max(math.floor(debugger.display / 3), 1))
+    if direction == "up":
+        debugger.src_offset -= debugger.src_offset_diff
+        print(debugger.print_source(debugger.display, debugger.src_offset))


Put the print functions after the else statement to eliminate the code duplication.

robertsipka · 2018-06-21T05:37:28Z

jerry-debugger/jerry_client.py

+
+    if args.client_source is not None:
+        if args.client_source != []:
+            prompt.debugger.store_client_sources(args.client_source)


No need for two if statements.

if args.client_source: prompt.debugger.store_client_sources(args.client_source)

robertsipka · 2018-06-26T13:00:39Z

jerry-debugger/jerry_client.py

+
+    prompt = DebuggerPrompt(debugger)
+    prompt.prompt = "(jerry-debugger) "
+    prompt.debugger.non_interactive = non_interactive


This non_interactive variable is unnecessary. You can use arg.non_interactive directly.

robertsipka · 2018-06-26T13:23:04Z

jerry-debugger/jerry_client_ws.py

+            sbp = self.breakpoint_info
+            self.breakpoint_info = ''
+            return sbp
+        return None


This would be prettier.

_set_breakpoint(self, args, False) if self.breakpoint_info == '': return None sbp = self.breakpoint_info self.breakpoint_info = '' return sbp

robertsipka · 2018-06-26T13:47:42Z

jerry-debugger/jerry_client_ws.py

+        if not args:
+            result = "Error: Status expected!"
+        else:
+            enable = int(args)


This will leads to a crash if I gave a non integer argument. Please handle it also.

robertsipka · 2018-06-27T08:47:15Z

jerry-debugger/jerry_client_ws.py

+        self.breakpoint_info = ''
+        return sbp
+
+


Please remove the extra line.

robertsipka · 2018-06-27T09:31:02Z

jerry-debugger/jerry_client_ws.py

+        else:
+            result = "Error: Invalid input! Usage 1: [Enable] or 0: [Disable]."
+        return result
+


Sorry for the nitpicks, but maybe you could rewrite this function to increase the readability.

def exception(self, args): try: enabled = int(args) except (ValueError, TypeError) as val_errno: return "Error: Positive integer number expected, %s" % (val_errno) if enabled not in [0,1]: return "Error: Invalid input! Usage 1: [Enable] or 0: [Disable]." if enabled: logging.debug("Stop at exception enabled") self.send_exception_config(enabled) return "Stop at exception enabled" logging.debug("Stop at exception disabled") self.send_exception_config(enabled) return "Stop at exception disabled"

zherczeg · 2018-06-28T03:20:46Z

jerry-debugger/jerry_client.py

+            except ValueError as val_errno:
+                print("Error: Positive breakpoint index expected: %s" % val_errno)
+        else:
+            sbreak = self.debugger.set_break(args)


I would do these checks on the other side (set_break), and return with a text. If the text is empty, don't print anything. Otherwise print the text.

zherczeg · 2018-06-28T03:22:29Z

jerry-debugger/jerry_client.py

+        """ Lists the available breakpoints """
+        if self.debugger.active_breakpoint_list:
+            print("=== %sActive breakpoints %s ===" % (self.debugger.green_bg, self.debugger.nocolor))
+            for breakpoint in self.debugger.active_breakpoint_list.values():


It would be better to use functions to return these values instead of directly access private variables. Please make these variables private.

zherczeg · 2018-06-28T04:12:37Z

jerry-debugger/jerry_client.py

+
+        result = prompt.debugger.mainloop()
+
+        if result == '':


I would like a result with a type and data fields, where data depends on the type. It can be a simple class.

zherczeg · 2018-06-28T04:13:07Z

jerry-debugger/jerry_client.py

+        if not args:
+            print("Error: Breakpoint index expected")
+            print("Delete the given breakpoint, use 'delete all|active|pending' to clear all the given breakpoints ")
+        elif args in ['all', 'pending', 'active']:


Please move all of these checks to debugger side.

zherczeg

LGTM

robertsipka

LGTM

robertsipka · 2018-07-13T07:41:25Z

@TamasZakor Please rebase it to the master.

rerobika

Great work just a minor suggestion.

rerobika · 2018-07-20T07:12:16Z

jerry-debugger/jerry_client_ws.py

    # pylint: disable=too-many-instance-attributes,too-many-arguments
    def __init__(self, is_func, byte_code_cp, source, source_name, line, column, name, lines, offsets):
-        self.is_func = is_func
+        self.is_func = True if is_func else False


If the bool cast is necessary I'd rather use self.is_func = bool (is_func).

Move DebuggerPrompt to jerry_client.py Implement JerryDebugger functions in the jerry_client_ws.py file Server response is displayed by jerry_client.py JerryScript-DCO-1.0-Signed-off-by: Tamas Zakor ztamas@inf.u-szeged.hu

zherczeg · 2018-07-23T06:17:50Z

Can we land this?

TamasZakor force-pushed the jerry-db-3 branch from 280eab2 to 53dc2ee Compare June 20, 2018 07:42

zherczeg requested changes Jun 20, 2018

View reviewed changes

TamasZakor force-pushed the jerry-db-3 branch 2 times, most recently from 36874be to d39cd04 Compare June 20, 2018 13:47

robertsipka requested changes Jun 21, 2018

View reviewed changes

TamasZakor force-pushed the jerry-db-3 branch 7 times, most recently from 479a8d1 to e545934 Compare June 25, 2018 07:32

robertsipka requested changes Jun 26, 2018

View reviewed changes

TamasZakor force-pushed the jerry-db-3 branch 2 times, most recently from bf793e9 to be9f4c2 Compare June 27, 2018 06:47

robertsipka requested changes Jun 27, 2018

View reviewed changes

TamasZakor force-pushed the jerry-db-3 branch from be9f4c2 to 3c893c3 Compare June 27, 2018 10:02

zherczeg requested changes Jun 28, 2018

View reviewed changes

TamasZakor force-pushed the jerry-db-3 branch 2 times, most recently from fdd1c71 to 56718fd Compare June 29, 2018 06:30

zherczeg approved these changes Jul 9, 2018

View reviewed changes

robertsipka approved these changes Jul 13, 2018

View reviewed changes

TamasZakor force-pushed the jerry-db-3 branch 7 times, most recently from 61677d6 to 36f3a22 Compare July 19, 2018 13:17

rerobika reviewed Jul 20, 2018

View reviewed changes

Splitting the debugger and console part of the python debugger

a459511

Move DebuggerPrompt to jerry_client.py Implement JerryDebugger functions in the jerry_client_ws.py file Server response is displayed by jerry_client.py JerryScript-DCO-1.0-Signed-off-by: Tamas Zakor ztamas@inf.u-szeged.hu

TamasZakor force-pushed the jerry-db-3 branch from 36f3a22 to a459511 Compare July 20, 2018 09:42

LaszloLango merged commit 8482fef into jerryscript-project:master Jul 23, 2018

zherczeg mentioned this pull request Mar 19, 2019

Nominating Robert Fancsik (@rerobika) to become a JerryScript reviewer #2799

Closed

Splitting the debugger and console part of the python debugger #2406

Splitting the debugger and console part of the python debugger #2406

Uh oh!

Conversation

TamasZakor commented Jun 20, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsipka Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertsipka Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

robertsipka left a comment

Choose a reason for hiding this comment

Uh oh!

robertsipka commented Jul 13, 2018

Uh oh!

rerobika left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Jul 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

robertsipka Jun 21, 2018 •

edited

Loading

robertsipka Jun 27, 2018 •

edited

Loading