Added repr and a helper function #10

KOLANICH · 2017-03-15T16:04:37Z

No description provided.

GreyCat · 2017-03-15T20:59:21Z

Thanks for the contribution!

Unfortunately, this code breaks many things:

First and foremost, it fails with an error:

     33 
     34     def __repr__(self):
---> 35         return "".join((self.__class__.__name__, "(", ", ".join(self._reprGeneratorForAllProps(self)), ")"))
     36 
     37     def __exit__(self, *args, **kwargs):

TypeError: _reprGeneratorForAllProps() takes exactly 1 argument (2 given)

If it was working, this code dumps only seq items, but it will ignore instances, as they are stored in _m_foo like members, starting with _, which you skip
Adding support for instances might be tricky, as you'll need to avoid problems with infinite recursion to be encountered in cases like these:

meta:
  id: rec
seq:
  - id: some
    type: u1
instances:
  self_ref:
    value: _root

Naming:
- function names must use lower underscore case
Complexity:
- cramming 4-5-6 statements into a single line is not a good idea
- Iadding extra function in KaitaiStruct class that will be inherited everywhere is probably not a good idea as well
Formatting:
- PEP8 requires usage of whitespace around some binary operators and recommends to use it around the others
- line length must be up 79 — you have 109 here
- docstring must end with a period
- Codacy already reported trailing whitespace

funkyfuture · 2017-03-23T11:13:32Z

kaitaistruct.py

@@ -27,6 +27,13 @@ def __init__(self, stream):
    def __enter__(self):
        return self

+    def _reprGeneratorForAllProps(self):


this name doesn't comply with pep8.

arekbulski · 2018-01-19T11:44:02Z

Forgive me for being outright dismissive but... is __repr__ really that much needed? @GreyCat I noticed that you and I run our forums completely differently. In Construct issues, I would have closed this PR after no more than a day of deliberation and half the arguments. You seem to keep issues open forever for everyone to see and participate. I have to admit, and very ahem deliberately, that I appreciate the way you run things. But lets close this one and move on? 🤔

KOLANICH · 2018-01-19T20:16:52Z

__repr__ is definitely needed. It is a pain to deal (especially do debug in ipython shell) with anything without __repr__. Feel free to fix that PR any way you like, I don't have time to work on it for now.

arekbulski · 2018-01-19T21:07:23Z

I just fixed the style/pep8, didnt change how it works. Better?

KOLANICH · 2018-03-20T18:57:25Z

I have added a guard against recursive objects.

KOLANICH · 2018-03-20T19:26:21Z

Broadened scope to instances. @arekbulski @GreyCat @funkyfuture could you test and review it, it seems it's ready.

funkyfuture · 2018-03-21T10:15:58Z

kaitaistruct.py

@@ -15,6 +15,18 @@
 #
 __version__ = '0.8'

+from RecursionSafe import RecursionSafe
+
+recSafe=RecursionSafe()


does flake8 not complain about that symbol name?

recSafe will be fixed to rec_safe. I don't use flake.

funkyfuture · 2018-03-21T10:22:00Z

kaitaistruct.py

+  """Generator to use in own __repr__ functions."""
+  return (
+    "".join(( str(k), "=", repr(getattr(self, k)) ))
+    for k in dir(self)


i don't know if it's usable in this context, but vars is often inferior over dir and might make test in the following line unnecessary.

vars doesn't return properties.

funkyfuture · 2018-03-21T10:22:41Z

kaitaistruct.py

+  return (
+    "".join(( str(k), "=", repr(getattr(self, k)) ))
+    for k in dir(self)
+    if k[0] != "_" and not hasattr(KaitaiStruct, k) and not isinstance(getattr(self, k), type)


k.startswith('_') would be more expressive.

It's longer and twice slower.

funkyfuture · 2018-03-21T10:23:06Z

kaitaistruct.py

@@ -26,6 +38,16 @@ def __enter__(self):
    def __exit__(self, *args, **kwargs):
        self.close()

+    def __repr__(self):
+        return "".join(


these are too much line-breaks for my gusto.

funkyfuture · 2018-03-21T10:25:11Z

setup.py

@@ -44,4 +44,6 @@
    ],
    keywords='kaitai,struct,construct,ksy,declarative,data structure,data format,file format,packet format,binary,parser,parsing,unpack,development',
    py_modules=["kaitaistruct"],
+    requires=["RecursionSafe"],
+    dependency_links=["git+https://github.com/KOLANICH/RecursionSafe.py.git"],


i wouldn't trust a python library with camelcase naming and .py suffix. ;-)

also, i wouldn't outsource 45 lines of code to create a dependency, but rather put it in an utils module.

Also:

Requirements
Python 3.

Which is, obviously, a deal-breaker for inclusion here anyway :(

Python 2 is dead, stop raping its corpse.

Unfortunately, I see the exact opposite :(

funkyfuture

my 2 cents.

arekbulski · 2018-03-21T14:31:26Z

I dont even understand how this code works, nor why its so lengthy. Also its not providing different str and repr versions. I would suggest considering using Construct as reference:

recursion lock
only 16 lines of code
https://github.com/construct/construct/blob/492b5f93679b6fb21f01558e20420efabc6dac01/construct/lib/containers.py#L29-L44
both STR and REPR
REPR produces a full one-liner that entirely reproduces the dict.
STR produces newline-separated shortened items (truncated to 32 bytes) for viewing.
https://github.com/construct/construct/blob/492b5f93679b6fb21f01558e20420efabc6dac01/construct/lib/containers.py#L207-L250

arekbulski · 2018-03-21T14:47:17Z

I appreciate the initiative, but honestly I dont like the implementation. If you would be willing to wait, then I will offer alternative implementation based on Construct.

arekbulski

I dont like this implementation overall, but I dont have any specific points to make.

KOLANICH · 2018-03-21T17:03:20Z

There were some errors, I have fixed them.

KOLANICH · 2018-03-29T20:30:00Z

@GreyCat @arekbulski I was advised with a good replacement for a lock and truncation which are already a part of python stdlib :)

KOLANICH · 2018-03-29T20:43:25Z

reprlib is missing on python 2, but I guess may be backported and put into pip, the name is free https://pypi.python.org/pypi/reprlib

arekbulski · 2018-03-30T03:48:59Z

I have issues with this implementation:

reprlib is Python 3 only but the runtime is supposed to be Python 2/3
your code uses different indentation than rest of the script
I dont particularly like the usage of dir, we could ask @GreyCat to change the compiler so that generated structs have something similar to __slots__, so you would just iterate that.
it evalutates unevaluated (lazy) instances? this could lead to failure.

KOLANICH · 2018-03-30T06:32:00Z

reprlib is Python 3 only but the runtime is supposed to be Python 2/3

Again: we can try to backport it and put into pip.

your code uses different indentation than rest of the script

fixed. Thank you for bringing it up.

I dont particularly like the usage of dir, we could ask @GreyCat to change the compiler so that generated structs have something similar to slots, so you would just iterate that.

__slots__ cannot be properties

it evalutates unevaluated (lazy) instances? this could lead to failure.

It does, it could. But reprlib provides recursion lock and truncation, so I guess there will be no infinite recursions and rendering too much values of sequences. About reading large files into memory which can cause consuming all the memory in machine - I guess we should map them, not cache them and use references to them instead of copies (I wonder if it is possible in python), this would lead into consuming only a single page. Maybe we need a volatile hint to optimize this.

arekbulski · 2018-03-30T07:06:27Z

__slots__ cannot be properties

But thats not what I said or meant. We could have the compiler generate KaitaiStruct-s that have something similar to slots, a list of names, to iterate that instead of filtering dir:

class Enum0(KaitaiStruct):
    __slotsalike__ = ["pet_1","pet_2"]
    def _read(self):
        self.pet_1 = self._root.Animal(self._io.read_u4le())
        self.pet_2 = self._root.Animal(self._io.read_u4le())

then

-        for k in dir(self)
-        if k[0] != "_" and not hasattr(KaitaiStruct, k) and not isinstance(getattr(self, k), type)
+    for k in __slotsalike__: 
+    yield getattr(self, k)

This would not only have better performance, if you care about that in this case that is, but it might also be useful to end users as they might have a use for a list of field names.

arekbulski · 2018-03-30T07:09:21Z

But reprlib provides recursion lock and truncation,

That does not address the issue I meant. You could have a pointer instance that refers to outside of stream size, which will cause failure if evaluated. Its not about the size of the output.

KOLANICH · 2018-03-30T09:16:05Z

You could have a pointer instance that refers to outside of stream size, which will cause failure if evaluated. Its not about the size of the output.

Out of scope - its responsibility of a ksy developer to have all the needed ifs.

But thats not what I said or meant. We could have the compiler generate KaitaiStruct-s that have something similar to slots, a list of names, to iterate that instead of filtering dir:

I agree. I guess we need non-volatile fields in _fields, properties in _props and everything possible in __slots__. And tuples should be used, not lists.

And I guess that collection of some of this stuff can be done in runtime without any changes to compiler.

arekbulski · 2018-04-01T08:23:45Z

@GreyCat Could you share your position with us? How about we have the compiler attach a list of IDs to each generated Struct, similar to slots?

GreyCat · 2018-04-01T08:34:00Z

Compiler actually already does something similar when doing --debug, but not for Python. I have no idea what you guys mean by "non-volatile fields in _fields, properties in _props and everything possible in __slots__", i.e. what is "non-volatile fields", "properties", "everything possible".

We generate an array called something like "seq_fields", i.e.:

Could you explain, what's wrong with current solution - i.e. is it's reflection slow, unreliable, or something else is the matter? As far as I have guessed, __slots__ is some special array in Python?

KOLANICH · 2018-04-01T08:42:43Z

I have no idea what you guys mean by "non-volatile fields in _fields, properties in _props and everything possible in __slots__", i.e. what is "non-volatile fields", "properties", "everything possible".

Non-volatile fields are the fields (members in seq) without volatile hint. _props is a tuple of names of everything implemented as @propertyes: instances, volatile fields, etc. __slots__ is a special collection listing the names of dumb (non-@property members), if it is present, only the names listed in it are allowed (unless you have some magic method overloading it), __dict__ is unavailable and it is claimed that memory footprint is reduced.

is it's reflection slow, unreliable, or something else is the matter

we process strings to determine if a name to be included, it's ugly, and may be slow

One more considiration: I guess we'll have inheritance somewhen, so the thing should be compatible to it without much overhead.

arekbulski · 2018-04-01T08:58:21Z

SEQ_FIELDS is exactly what I am talking about. The only issue with it is that it should be included even without debug mode and available in Python.

On Kolanich behalf: non-volatile is just a synonym for cached (like instances are cached).

z3ntu · 2020-11-04T23:06:23Z

Hi, what's the state of this PR? I'm interest in this as well as the default string representation is not really useful at the moment <MyBinaryType object at 0xb18605b0> (and I wanted to spare myself writing it myself for a quick prototype 😉)

KOLANICH · 2020-11-05T07:44:11Z

The status of this PR is that I personally use it, but in a form of a separate branch on top of master. On large files it may cause problems, since text is generated first, then is output, we don't evaluate size of the text that will be generated before generating it.

funkyfuture reviewed Mar 23, 2017

View reviewed changes

KOLANICH force-pushed the __repr__ branch from 8ee34f3 to 7c38b83 Compare March 20, 2018 18:56

KOLANICH force-pushed the __repr__ branch from 7c38b83 to ee93129 Compare March 20, 2018 19:25

funkyfuture reviewed Mar 21, 2018

View reviewed changes

GreyCat requested review from GreyCat and arekbulski March 21, 2018 10:40

KOLANICH force-pushed the __repr__ branch 3 times, most recently from 8c117dd to 9d712b1 Compare March 21, 2018 11:56

GreyCat mentioned this pull request Mar 21, 2018

Debug print (toString / str / to_s / repr / inspect / etc) implementations kaitai-io/kaitai_struct#299

Open

arekbulski approved these changes Mar 21, 2018

View reviewed changes

KOLANICH force-pushed the __repr__ branch from 9d712b1 to 62cbfa9 Compare March 21, 2018 17:01

KOLANICH force-pushed the __repr__ branch 4 times, most recently from 8192392 to d4aee68 Compare March 22, 2018 19:49

KOLANICH force-pushed the __repr__ branch from e895a98 to 8983411 Compare March 29, 2018 20:32

KOLANICH force-pushed the __repr__ branch from 8983411 to add2ed6 Compare March 30, 2018 06:35

GreyCat mentioned this pull request Mar 12, 2019

How to determine all fields to descend into? kaitai-io/kaitai_struct#531

Open

KOLANICH force-pushed the __repr__ branch from add2ed6 to 5ef5f21 Compare May 7, 2019 17:11

KOLANICH force-pushed the __repr__ branch from 5ef5f21 to 03cd8ab Compare January 23, 2020 16:00

KOLANICH force-pushed the __repr__ branch from 03cd8ab to 9bf39e9 Compare February 12, 2020 16:52

KOLANICH force-pushed the __repr__ branch from 9bf39e9 to 63e0b58 Compare February 27, 2020 22:54

KOLANICH force-pushed the __repr__ branch from 63e0b58 to d19bf16 Compare June 11, 2020 14:34

KOLANICH mentioned this pull request Sep 21, 2020

Python API and docs #51

Open

KOLANICH force-pushed the __repr__ branch from d19bf16 to 581a5f7 Compare November 16, 2020 13:17

KOLANICH force-pushed the __repr__ branch from 581a5f7 to 79f5669 Compare December 15, 2020 17:31

KOLANICH force-pushed the __repr__ branch 3 times, most recently from 38e38f3 to bbad79a Compare April 12, 2022 10:29

KOLANICH force-pushed the __repr__ branch from bbad79a to 573c01b Compare July 25, 2022 08:05

Added __repr__ and a helper function

12b4ee7

KOLANICH force-pushed the __repr__ branch from 573c01b to 12b4ee7 Compare March 12, 2023 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added repr and a helper function #10

Added repr and a helper function #10

KOLANICH commented Mar 15, 2017

GreyCat commented Mar 15, 2017

funkyfuture Mar 23, 2017

arekbulski commented Jan 19, 2018

KOLANICH commented Jan 19, 2018 •

edited

Loading

arekbulski commented Jan 19, 2018

KOLANICH commented Mar 20, 2018

KOLANICH commented Mar 20, 2018 •

edited

Loading

funkyfuture Mar 21, 2018

KOLANICH Mar 21, 2018

funkyfuture Mar 21, 2018

KOLANICH Mar 21, 2018

funkyfuture Mar 21, 2018

KOLANICH Mar 21, 2018 •

edited

Loading

funkyfuture Mar 21, 2018

funkyfuture Mar 21, 2018 •

edited

Loading

GreyCat Mar 21, 2018

funkyfuture left a comment

arekbulski commented Mar 21, 2018

arekbulski commented Mar 21, 2018

arekbulski left a comment

KOLANICH commented Mar 21, 2018

KOLANICH commented Mar 29, 2018 •

edited

Loading

KOLANICH commented Mar 29, 2018 •

edited

Loading

arekbulski commented Mar 30, 2018

KOLANICH commented Mar 30, 2018 •

edited

Loading

arekbulski commented Mar 30, 2018

arekbulski commented Mar 30, 2018

KOLANICH commented Mar 30, 2018 •

edited

Loading

arekbulski commented Apr 1, 2018

GreyCat commented Apr 1, 2018

KOLANICH commented Apr 1, 2018 •

edited

Loading

arekbulski commented Apr 1, 2018

z3ntu commented Nov 4, 2020

KOLANICH commented Nov 5, 2020

Added __repr__ and a helper function #10

Are you sure you want to change the base?

Added __repr__ and a helper function #10

Conversation

KOLANICH commented Mar 15, 2017

GreyCat commented Mar 15, 2017

Choose a reason for hiding this comment

arekbulski commented Jan 19, 2018

KOLANICH commented Jan 19, 2018 • edited Loading

arekbulski commented Jan 19, 2018

KOLANICH commented Mar 20, 2018

KOLANICH commented Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KOLANICH Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyfuture Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyfuture left a comment

Choose a reason for hiding this comment

arekbulski commented Mar 21, 2018

arekbulski commented Mar 21, 2018

arekbulski left a comment

Choose a reason for hiding this comment

KOLANICH commented Mar 21, 2018

KOLANICH commented Mar 29, 2018 • edited Loading

KOLANICH commented Mar 29, 2018 • edited Loading

arekbulski commented Mar 30, 2018

KOLANICH commented Mar 30, 2018 • edited Loading

arekbulski commented Mar 30, 2018

arekbulski commented Mar 30, 2018

KOLANICH commented Mar 30, 2018 • edited Loading

arekbulski commented Apr 1, 2018

GreyCat commented Apr 1, 2018

KOLANICH commented Apr 1, 2018 • edited Loading

arekbulski commented Apr 1, 2018

z3ntu commented Nov 4, 2020

KOLANICH commented Nov 5, 2020

Added repr and a helper function #10

Added repr and a helper function #10

KOLANICH commented Jan 19, 2018 •

edited

Loading

KOLANICH commented Mar 20, 2018 •

edited

Loading

KOLANICH Mar 21, 2018 •

edited

Loading

funkyfuture Mar 21, 2018 •

edited

Loading

KOLANICH commented Mar 29, 2018 •

edited

Loading

KOLANICH commented Mar 29, 2018 •

edited

Loading

KOLANICH commented Mar 30, 2018 •

edited

Loading

KOLANICH commented Mar 30, 2018 •

edited

Loading

KOLANICH commented Apr 1, 2018 •

edited

Loading