aurzenligl / prophy

prophy: fast serialization protocol
MIT License
13 stars 8 forks source link

Prophy python codec cleanup #16

Closed kamichal closed 5 years ago

kamichal commented 6 years ago

A step ahead to:

aurzenligl commented 6 years ago

I'll need a while to read into it, most probably on weekend.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.05%) to 99.336% when pulling 7a2e7cd9f249db174825da29bc802d781f2a65f2 on wire_interface_versioning into 1c60fa2c6798dce437e3cdac99d4e726da581553 on master.

aurzenligl commented 6 years ago

The refactoring of Python codec is a very good idea, things got cleaner now. I have a couple of suggestions regarding placement of functionalities throughout modules.

I'd probably move base_array to container.py. I'd probably move (keep?) enum to scalar.py. I'd probably retain composite.py name instead of data_types.py which doesn't define all data types. I'd probably rename desc_item.py to descriptor.py, and store all descriptor-related things there (reflection API, etc.). I'd probably rename descriptor_item_type to field which sounds simpler and to the point (statement "descriptor is composed of fields" rings the bell). I'd probably move codec_kind to descriptor.py - since that's the place with things common to all data types.

All in all - separation or generators and descriptors from big composite blob is definitely an improvement.

kamichal commented 6 years ago

Sometimes I bother with class names. There are 38 class definitions (excluding tests) in prophy package and only ProphyError is written in camel case. You asked me to rename descriptor_item_type to field, but that's a common variable name used massively in the package, so it collides.

I can either to call it e.g. field_type or use the PEP8 convention, i.e.: Field. Of course that breaks the convention and even PEP8 recommends keeping existing convention.

So I would like to as to break the convention and rename the classes to CamelCase in whole prophy package. Of course external interface (prophy usage) will not change, because prophy.__init__.py allows to rename exported definitions, (e.g. from .generators import EnumGenerator as enum_generator).

Do you, Krzysztof want me to do that? :dancer:

aurzenligl commented 6 years ago

I can either to call it e.g. field_type or use the PEP8 convention, i.e.: Field. Of course that breaks the convention and even PEP8 recommends keeping existing convention.

That's definitely OK. I don't care whether it's field_type or field or Field or FieldType, I'd just want the core part of this type name to be some form of field word :D

So I would like to as to break the convention and rename the classes to CamelCase in whole prophy package. Of course external interface (prophy usage) will not change, because prophy.init.py allows to rename exported definitions, (e.g. from .generators import EnumGenerator as enum_generator).

Both ways are ok - all in snake case (against PEP8) or all in CamelCase (aligned to PEP8). I agree that keeping backwards compatibility by exposing small case is a good idea.

Do you, Krzysztof want me to do that?

If you feel like it - then do it ;) Code will become yet more clear.

kamichal commented 5 years ago

I decided to discontinue the "wire interface versioning" implementation on this side (in prophy) and to implement it in prophyc. I removed the fishy part with descriptor iterator (called like "wire bricks walk"). So this pull request consists only of refactoring.

kamichal commented 5 years ago

Thanks. It's ready to be merged. Version bump and release can wait for "babel part 1".