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

Message class #51

Closed
banksJeremy opened this issue May 3, 2014 · 8 comments
Closed

Message class #51

banksJeremy opened this issue May 3, 2014 · 8 comments
Assignees

Comments

@banksJeremy
Copy link
Collaborator

(related to some discussion in #43)

We should have a Message class. A MessagePosted event's .message and the corresponding UserMentioned event's .message should both refer to the same Message instance. If a MessageEdited event occurs, it would update this shared instance.

The messages could be de-duplicated using a weak-key dictionary on the Message class.

We will also add a bounded deque (perhaps max_len = 1000) of .recent_events on wrapper, which keep recent Message instances alive, when they're most likely to be referenced again. (Otherwise we could end up creating and destroy several message instances for different events (UserMentioned, MessageEdited) that are really in response to the same action, and we wouldn't be able to automatically aggregate information from multiple associated events.)

Message objects will only use data from events received from rooms we're watching; we aren't going to have any properties which make new requests to the server to retrieve missing data. For all unknown fields (e.g. if we haven't seen an event that tells us the start count of a message), the value will just be None.

The .reply() method on MessageEvent will be moved to Message.

It will also have a .star(value=True) method (see #50).

@Manishearth
Copy link
Owner

So in essence the MessageEvent class gets converted to a shell that contains a Message object and reexports some Message methods?

@banksJeremy
Copy link
Collaborator Author

It probably wouldn't even re-export any methods. You'd need to do event.message.reply(foo).

Having a distinct, de-duplicated Message object would let you do things like:

def on_event(event, wrapper):
    if isinstance(event, events.MessageStarred):
        print event.message.owner_name, "has a message starred!"

which would currently be impossible because the user_name associated with the MessageStarred event is the user doing the starring, not the owner of the starred message. This new approach would have that information available as long as we had previously observed.

Similarly, you would be able to easily:

    print "in reply to:", event.message.parent.text_content

instead of needing to keep track of the messages to find the one to associate with the parent_id yourself.

@Manishearth
Copy link
Owner

This would be nice.

We'd need to be able to fetch messages if they're not cached, though -- maybe by fetching the history. (there may be a json endpoint, too).

I'll look into this tomorrow.

-----Original Message-----
From: "Jeremy Banks" notifications@github.com
Sent: ‎5/‎4/‎2014 11:02 AM
To: "Manishearth/ChatExchange" ChatExchange@noreply.github.com
Cc: "Manish Goregaokar" manishsmail@gmail.com
Subject: Re: [ChatExchange] Message class (#51)

We might also want a message.watch(on_change) method, which can be used to be notified when any properties on a message have changed, in case you need to re-render it in a client, for example.

Reply to this email directly or view it on GitHub.

@banksJeremy banksJeremy removed their assignment May 4, 2014
banksJeremy pushed a commit that referenced this issue May 5, 2014
Made .wrapper a mandatory attribute of Event.

Updates tests a bit (probably need more work) to ensure that messages
are deduplicated between Event instances. We access the content of a
message's parent, which is only available because it's been cached.

ref #51
@banksJeremy
Copy link
Collaborator Author

I've implemented the basic Message class described above, without any capability to automatically fetch missing data (fields are left as None for now). The Message is just built out of information from Events seen by a wrapper.

We'll need to test different events to find out what the user_id represents for each. If edit events represent the editor, not the owner, then we should use those edit events to update .editor_user_name and .editor_user_id properties.

banksJeremy pushed a commit to jeremyBanks/stack.chat that referenced this issue May 5, 2014
banksJeremy pushed a commit that referenced this issue May 5, 2014
@Manishearth
Copy link
Owner

r+, if you're waiting for my review, otherwise, carry on :)

@banksJeremy banksJeremy self-assigned this May 5, 2014
@banksJeremy
Copy link
Collaborator Author

I was hoping for a review to make sure I wasn't doing anything disagreeable, but I'm not finished yet. :)

I'm adding a simple web-based chat viewer example, which I think will make a good way to show changes to previous Messages, when they're edited or whatever.

banksJeremy pushed a commit that referenced this issue May 5, 2014
This now reflects changes to messages (edits, starrings, etc.)

ref #51
@banksJeremy
Copy link
Collaborator Author

Keeping in mind your comments on #43...

Basically, I envision something like this:

def init(self):
  self.gettergroups={"access":False,"info":False,...}
  self.info()
def access(self):
  # do some GETting
  # initialize related params

@lazy('access','owner')
@property
def owner(self):
  "NOP" #The decorator does the magic.
#etc etc

I've implemented a pretty simple accessor for us to use when implementing lazy lookup of missing values.

class Message(object):
    # ...

    content_source = LazyFrom('_scrape_source')

    def _scrape_source(self):
        self.content_source = self.wrapper.getSomething(
            '/mesages/%s?plain=true' % (self.message_id,))

    room_id = LazyFrom('_scrape_timeline')
    content = LazyFrom('_scrape_timeline')

    def _scrape_timeline(self):
        # ...
        self.content = ...
        self.room_id = ...

The update methods just assign to the attributes as normal, the only difference is that when a attribute is missing, we invoke a specified method to populate it. Our Event code can continue setting/updating known attributes on their message instances, as they have been.

Here's the current implementation of LazyFrom:

class LazyFrom(object):
    """
    A accessor used when multiple lazy attributes depend on a common
    source of data.
    """
    def __init__(self, method_name, backing_attr_name):
        """
        method_name is the name of the method that will be invoked if
        the value is not known. It must asign a value for the attribute
        attribute (through this accessor).
        """
        self.method_name = method_name
        self.values = weakref.WeakKeyDictionary()

    def __get__(self, obj, cls):
        if obj is None:
            raise AttributeError()

        if obj not in self.values:
            method = getattr(obj, self.method_name)
            method()

        return self.values[obj]

    def __set__(self, obj, value):
        self.values[obj] = value

    def __delete__(self, obj):
        del self.values[obj]

Does this seem like an reasonable approach, @Manishearth?

@Manishearth
Copy link
Owner

This seems to be a good way of doing it. I find descriptors a bit annoying to work with, but that's a personal preference :)

banksJeremy pushed a commit to jeremyBanks/stack.chat that referenced this issue May 6, 2014
…cessed.

Adds a test which looks up messages by ID, without logging in.

Adds methods to scrape transcript, and to scrape markdown source of
messages.

Renamed test_live_messages -> test_wrapper.

Upgrades to BeautifulSoup 4, for use of the .select() method.

ref Manishearth#51
banksJeremy pushed a commit to jeremyBanks/stack.chat that referenced this issue May 6, 2014
banksJeremy pushed a commit that referenced this issue May 6, 2014
History scraping is still most unimplemented.

resolves #50, ref #51
banksJeremy pushed a commit that referenced this issue May 13, 2014
I'm going to ignore time_stamp for now.

ref #51
banksJeremy pushed a commit that referenced this issue May 13, 2014
I'm going to ignore time_stamp for now.

ref #51
@banksJeremy banksJeremy mentioned this issue May 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants