Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dvl/klaus/webadm readable msg #85

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

klausfmh
Copy link
Collaborator

The goal is to have adapt the web interface such, that the messages that are posted are a little better readable.

Copy link
Collaborator

@jrmi jrmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review not yet finished.

@@ -455,6 +457,28 @@ def graph_dot(self, end=''):
for end, sub in after:
sub.graph_dot(end=end)

def jsonable_msg_info_for_admin(self, msg):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is not a Message method ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each channel handles different kinds of messages.
I imagine the usecase where a channel wants to have a different representation of a message.

Let's say an MLLP channel would prefer to have perhaps a partially preparsed representation for the web interface,
A file watcher channel might not display the contents of the file, but more stats about the file.

An alternative would of course be to put this method into the channel and let certain endpoints / channels subclass the Message object. (Though this might imply more data copies)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fair.

New proposal : you add the method to the Message class but you allow to redefine default payload_encoder with an argument. Then you add a method to the channel that call the Message method with the appropriate encoder. Later (i.e. in another PR) we can add a way to custom encoder by a channel argument.

What do you think ?

I think you can use current .to_dict() method and add the correct argument to achieve that ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After more thinking, more details :

  • You can create two messages serializer (at least) in the serializer file : b64picklerEncoder and JsonableEncoder,
  • Add payload_encoder option to Message.to_json() method with b64picklerEncoder as default encoder (to keep compatibility) and propagate the option to .to_dict() method,
  • Add a message_payload_encoder property to channel with b64 default encoder with default value to b64encoder,
  • Use this encoder in the .list_message() while calling .to_json() to get your formatted message.

Do I miss something ?

Copy link
Collaborator

@feenes feenes Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the param for message_payload_encoder be
a class, an instance (which must have the method encode or just a function that encodes).

Will make a first start with an instance, but I am open.

and the more I think the less I wonder why not just passing a function, except we want to group the encode and decode function in the same office.

In that case we should perhaps call the param
payload_codec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. pushed only to local gitlab server and not to github

Just check klausfmh@297b219

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact best to recheck all the deltas of this MR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder to continue review :-)
I think I handled all the FB. pls correct me if wrong

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's what i think for message but part :

  • Add a message_payload_encoder property to channel with b64 default encoder with default value to b64encoder,
  • Use this encoder in the .list_message() while calling .to_json() to get your formatted message.

is not yet done ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think jsonable_msg_info_for_admin channel method should not exists anymore at the end.

pypeman/channels.py Outdated Show resolved Hide resolved
pypeman/helpers/serializers.py Outdated Show resolved Hide resolved
pypeman/channels.py Show resolved Hide resolved
pypeman/helpers/serializers.py Outdated Show resolved Hide resolved
pypeman/message.py Show resolved Hide resolved
pypeman/msgstore.py Outdated Show resolved Hide resolved
pypeman/remoteadmin.py Outdated Show resolved Hide resolved
"""
List first `count` messages from message store of specified channel.

:params channel: The channel name.
:param mk_b64pickle: if True (yield payload / ctx fields as b64
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we create an payload_serializer param that can be what we want ? The default one is a b64pickle, but we can specify the "jsonable" one ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an option.

Another one would be to just pass a 'name' of a serializer and allow to register other ones if desired.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what to do?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with your proposal.

res['message'] = res['message'].to_json()
else:
msg = res['message']
res['message'] = chan.jsonable_msg_info_for_admin(msg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why is this not a message method ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above

pypeman/channels.py Outdated Show resolved Hide resolved
Repository owner deleted a comment from jrmi Apr 29, 2018
@klausfmh klausfmh force-pushed the dvl/klaus/webadm_readable_msg branch from 72864c9 to 4a8053d Compare May 4, 2018 15:42
@klausfmh klausfmh force-pushed the dvl/klaus/webadm_readable_msg branch 2 times, most recently from 1309368 to 680517e Compare November 30, 2018 17:09
@klausfmh klausfmh force-pushed the dvl/klaus/webadm_readable_msg branch from 680517e to e83e63b Compare January 3, 2019 16:49
@klausfmh klausfmh force-pushed the dvl/klaus/webadm_readable_msg branch from e83e63b to e62b4f7 Compare January 25, 2019 17:24
@klausfmh klausfmh force-pushed the dvl/klaus/webadm_readable_msg branch from e62b4f7 to b2d4706 Compare October 19, 2019 15:09
@klausfmh klausfmh force-pushed the dvl/klaus/webadm_readable_msg branch from b2d4706 to 6de03e9 Compare January 17, 2020 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants