Refactored functions creation

[KOLANICH]

No description provided.

[GreyCat]

Any comments on what this does exactly, why is it better than existing stuff?

[KOLANICH]

Any comments on what this does exactly, why is it better than existing stuff?

The old stuff creates methods manually. The new stuff is decomposed on endianness, variable type and action type. The methods are created by combining type, endianness and action. Benefits are obivious.

[GreyCat]

Benefits are obivious.

I see actually no benefits so far:

• it is much more complex
• it breaks any possible attempts to add docstrings (and proper documentation) to .py source
• I heavily suspect that it is slower on initialization (or may be even in run-time) than pre-rolled list of methods, and you've provide no claims/benchmarks that it stays the same
• it adds 2 complaints on Codacy to the codebase

[KOLANICH]

* it is much more complex

In fact it is not.

* it breaks any possible attempts to add docstrings (and proper documentation) to .py source

Self-documenting declarative code is better than docstrings and comments. Docstrings for generated functions are also generated.

I heavily suspect that it is slower on initialization

It is. A bit.

(or may be even in run-time) than pre-rolled list of methods, and you've provide no claims/benchmarks that it stays the same

Python is not for speed. It's slow by itself. cpython, the reference impl, has no jit-compilation, it is interpreter and pypy does jit-compilation only for hot-loops. So I see no reason why should that be slower enough to reject this kind of solution.

it adds 2 complaints on Codacy to the codebase Unused pack imported from struct Trailing whitespace

lol. They can be eliminated.

[GreyCat]

@dgladkov @felixonmars @antonv6 @koczkatamas Anyone can take a look at this PR and share some thoughts? May be I'm missing something important?

[antonv6]

This kind of refactoring would be worth doing if KaitaiStruct suddenly needed a bunch of new read_bla functions. I don't follow this project close enough to know if that's the case, but I somehow doubt it.

The proposed code is more complicated (e.g. adds metaclasses), not very pythonic, "features" type hints that don't work on 2.7 (I even think they cause SyntaxErrors, do the tests pass?) and subpar code style, also this commit/PR has unrelated changes. If anybody wants to know my opinion, let's say I'm not entirely sold on this.

[koczkatamas]

I think if we are using a compiler-based code generation, our main concern should be speed. There are faster python variants where our current implementation could perform much better than the proposed one. (I didn't make any speed tests or whatsoever, so it's just a hunch.)

So without convincing comparisons (including speed) between the current and proposed code I don't fancy merging this.

Although even if we don't accept the PR, @KOLANICH could support his own runtime if it serves his goals better and we could list it on a 3rd-party collaborations page, so others could also find it. But we cannot provide any support for that.