Move some internal functions into JerryDebugger.

[zherczeg]

The jerry_client_ws.py script should not have any public symbols except the Debugger class.

[robertsipka]

LGTM

[akosthekiss]

If these functions are not to be called from outside of the class, then python coding guidelines suggest prefixing them with either one or two underscores, making them 'protected' or 'private' (aka weakly and strongly internal), respectively. Right now, this patch is actually exposing these functions rather than hiding them (it is turning protected module-level functions into public class methods).

[zherczeg]

so these should be def _parse_source(self, data) ? Is this a guideline or enforced in some way?

[akosthekiss]

Yes, this is better called either _parse_source or __parse_source. If it is a non-public API of the class. These guidelines come from PEP8, the style guide for python. (As most things in Python, however, they are not enforced, only suggested. But the proper use of visibility is hard to enforce in any language, actually.)

Some excerpts from PEP8:

• _single_leading_underscore: weak "internal use" indicator. E.g. from M import * does not import objects whose name starts with an underscore.
• __double_leading_underscore: when naming a class attribute, invokes name mangling (inside class FooBar, __boo becomes _FooBar__boo; see below).
• Public attributes should have no leading underscores.

[zherczeg]

Thank you. Changed back to single underscore.

[akosthekiss]

Is the commit message intended as is? ("script should not have any public symbols except the Debugger class"). Right now, the module has a lot of public symbols in addition to JerryDebugger: a lot of constants, the arguments_parse function, and various classes. The commit content looks good to me but the commit message is misleading.

[zherczeg]

You are right. This is where you start disliking python. I moved the constant and added self._ prefixes.

[akosthekiss]

I don't think that python should be judged based on sentiments. There are quite often elegant ways to solve things in that language, too. E.g., the above cited PEP8 also gives suggestions for the current case.

Modules that are designed for use via from M import * should use the __all__ mechanism to prevent exporting globals, or use the older convention of prefixing such globals with an underscore (which you might want to do to indicate these globals are "module non-public").

Module level "dunders" (i.e. names with two leading and two trailing underscores) such as __all__, __author__, __version__, etc. should be placed after the module docstring but before any import statements except from __future__ imports.

I.e., in this case, prefixing all non-public globals with _ or defining __all__ = ['arguments_parse', 'JerryDebugger'] can both do the 'hiding' with significantly less code churn.

[zherczeg]

In this case from is not used.

[akosthekiss]

In this case from is not used.

Brevity doesn't help the discussion in this case.

• from is actually used in from __future__ statements.
• If you mean that from jerry_client_ws import * is not used in jerry_client.py, that's true. But the existing import jerry_client_ws could be rewritten like that to make jerry_client_ws. prefixing unnecessary. (And if this is not the end goal then what is the reason for all the rewrites in this PR?)
• If __all__ is strongly disliked, the _ prefixing of all non-public module level globals is still an option with less code churn (moving constants and classes inside JerryDebugger becomes unnecessary just like a lot of self. prefixes)

[zherczeg]

As far as I know if something is only used by a single class it should be part of that class, at least in C to reduce the chance of symbol clashes.

Anyway I moved the constants out from the debugger class, because pylint complained about a few of them. It said their format is incorrect without specifying why. Not exactly helpful.

Btw are these constants are safe or the application imports them can damage them?

[akosthekiss]

C doesn't have classes. C++ has them. (As well as types within types, namespaces, etc.) And both in C and in C++, there is the concept of splitting the interface and the implementation into headers and sources. In the headers, yes, one usually follows such guidelines as you've mentioned above for the sake of OOP. In the sources, one may use implementation-specific global constants or helper functions internal to the module but not member of any class, if needed.

Although Python does not split modules into two, it has solutions for defining what's public interface and what's implementation detail of a package, module, or class. Both __all__ and underscore prefixing can be used for 'hiding' entities in a module. Those things that are not part of the public API, can be whatever is good for the implementation.

The real question is, IMO, what's the ultimate goal of the rewrite? To make the jerry_client_ws module reusable? For whom and in what way? If it is used via import jerry_client_ws, then all its contents are available, even those that are underscore-prefixed or not listed in __all__, as this is Python after all. If it is used via from jerry_client_ws import *, then differentiating between public and internal items can be a good idea. But do we expect the module to be used by others outside this project / directory? (Right now, it is only jerry_client.py that's using this module.)

Some extras:

• Generally, it's strange to declare data structures like a Multimap inside another class. They are too general for that, even if they happen to be used in a single class in a given project.
• It could help in the structuring of the classes / types if JerryDebugger was architected with an additional transport layer in mind. What is WS-specific and what is transport layer independent? What should be shared between transport implementations?
• Why is there an argument parser in a module that's not executable as a command line script?
• Prefixing everything with Jerry, or JERRY_, or JERRY_DEBUGGER is strange in Python, as this is not C with a single flat namespace. In Python, each module has its own namespace and the import directives give direct control over what is pulled into a namespace from other modules.

As to your question "are these constants are safe or the application imports them can damage them?": I'm not completely sure what you mean. If another module uses from jerry_client_ws import *, then non-underscored globals will be imported into its scope. Whether this can "damage" the importing module is quite questionable. It can conceal programming errors in some weird cases but that's not frequent.

Based on all the above, my position on this rewrite is:

• Moving the _parse_source, _release_function, _enable_breakpoint, _set_breakpoint, _get_breakpoint global functions into JerryDebugger is a good step. They should really be methods of that class, which is also revealed by their first argument (debugger, to be rewritten as self).
• Moving jerry-specific classes (JerryBreakpoint, JerryPendingBreakpoint, JerryFunction, DisplayData) inside JerryDebugger is questionable. It can be done but it doesn't help readability. And they are quite standalone, they don't access anything within JerryDebugger, so the move is not that much justified.
• Moving a general data structure like Multimap inside JerryDebugger is something I'd strongly advise against.
• If the separation between public and internal entities is really needed, __all__ is way easier than prefixing everything with underscores.
• The first (or second?) version of the PR was almost perfect, since then a lot of unnecessary churn happened.

[zherczeg]

I only wanted to move those functions in the original code, but then the underscore issue was raised. I will go back to the original code then.

[akosthekiss]

Wait. The "underscore issue" was raised for the unnecessary renaming of _parse_source to parse_source. The unnecessary churn happened when the imprecision of the commit message was pointed out. At that point, my comment was: "The commit content looks good to me but the commit message is misleading."

[zherczeg]

Patch rolled back to the original version.

[akosthekiss]

LGTM