Polyconseil / aioamqp

AMQP implementation using asyncio
Other
280 stars 88 forks source link

Relecture partie "frame" #1

Closed tchaumeny closed 9 years ago

tchaumeny commented 10 years ago

https://github.com/Polyconseil/aioamqp/blob/bcz/connect_to_amqp/aioamqp/frame.py#L53 => instancier un AmqpEncoder à l'intérieur de l'AmqpEncoder me semble louche. Pourquoi ne pas travailler directement avec l'instance en cours ? sinon, peut-être envisager une classmethod.

https://github.com/Polyconseil/aioamqp/blob/bcz/connect_to_amqp/aioamqp/frame.py#L78 => ValueError ?

https://github.com/Polyconseil/aioamqp/blob/bcz/connect_to_amqp/aioamqp/frame.py#L84 => Plutôt qu'un cast :

if bit:
    byte_value |= (1 << arg_index)

Par ailleurs, ça va écrire les bits de droite à gauche (à l'inverse du sens dans lequels ils sont donnés). Il faudrait sans doute aussi vérifier le nombre de bits donnés.

https://github.com/Polyconseil/aioamqp/blob/bcz/connect_to_amqp/aioamqp/frame.py#L94 => Attention, ça va être endianess-dependant. Pour être indépendant de l'archi sous-jacente utiliser un "<" dans la chaîne de format (pareil pour les fonctions en-dessous):

>>> struct.pack("<H", 1)
b'\x01\x00'
>>> struct.pack(">H", 1)
b'\x00\x01'

https://github.com/Polyconseil/aioamqp/blob/bcz/connect_to_amqp/aioamqp/frame.py#L105 => Ne faudrait-il pas préciser un encoding pour un comportement prévisible ?

https://github.com/Polyconseil/aioamqp/blob/bcz/connect_to_amqp/aioamqp/frame.py#L119 => Même remarque que sur le write_table, je ne comprends pas pourquoi on instancie un autre encodeur.

https://github.com/Polyconseil/aioamqp/blob/bcz/connect_to_amqp/aioamqp/frame.py#L195 => Mêmes remarques sur l'endianess. Il faut garantir un comportement indépendant de la machine.

https://github.com/Polyconseil/aioamqp/blob/bcz/connect_to_amqp/aioamqp/frame.py#L218 => Ça ne retourne rien ?