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

API Design - Request for feedback #526

Open
madsmtm opened this issue Feb 5, 2020 · 21 comments
Open

API Design - Request for feedback #526

madsmtm opened this issue Feb 5, 2020 · 21 comments

Comments

@madsmtm
Copy link
Member

madsmtm commented Feb 5, 2020

Hi everyone, I've been working on v2 for a while now, and it's getting close to a point where I can start making alpha releases!

However, reworking this has been a mostly one-man effort, and I'd really like some input on how these changes are going to suit everyone else (after all, you're probably using this library more than I am 😁). Additionally, if you need me to elaborate on something, or explain the reasoning behind these API choices, I'd be more than happy to!

You can also use this issue as a way of requesting features, if there's something, important or not, we're missing!

A quick primer of major changes before we begin:

  • No Python 2 support
  • Type hints
  • Better listening system (no more error-prone subclassing)
  • Cleaner way of interracting with threads
    • client.changeThreadColor(color, thread_id=thread_id) -> thread.set_color(color)
  • async/await, backed by asyncio - Not yet implemented, follow asyncio fbchat #411 for more

Interracting with threads

Digging a bit further into the details, I'd like input on the changed API when interracting with threads. Previously, you'd call a method in Client with a thread ID and a ThreadType. This was pretty error prone, and involved a lot of extra typing. Now you simply create a Group/User object (or more often, fetch it from somewhere), and call methods on that.

An example:

# Old
import fbchat
client = fbchat.Client("<email>", "<password>")
print("Own id: {}".format(client.uid))

client.send(fbchat.Message(text="Hi me!"), thread_id=client.uid, thread_type=fbchat.ThreadType.USER)

client.send(fbchat.Message(text="Hi group!"), thread_id="<group id>", thread_type=fbchat.ThreadType.GROUP)

# New
import fbchat
session = fbchat.Session.login("<email>", "<password>")
print("Own id: {}".format(session.user.id))

session.user.send_text("Hi me!")

group = fbchat.Group(session=session, id="<group id>")
group.send_text("Hi group!")

Note also how we have pretty clear separation of concerns (Session handles logging in, Group handles interracting with the group).

Listening

Regarding listening, you'd previously override Client, and provide a custom implementation of an onX method. This was pretty error prone, and made it difficult for me to add new features. Now you create a Listener object, and register methods based on type hints.
How this is going to work exactly is very much up for discussion, so please comment on this!

An example:

# Old
import fbchat

class CustomClient(fbchat.Client):
    def onMessage(self, author_id, message, thread_id, thread_type, **kwargs):
        # If you're not the author, echo
        if author_id != self.uid:
            self.send(fbchat.Message(text=message), thread_id=thread_id, thread_type=thread_type)

client = CustomClient("<email>", "<password>")
client.listen()

# New
import fbchat
session = fbchat.Session.login("<email>", "<password>")
listener = fbchat.Listener(session=session, ...)  # TODO: Addition parameters

@listener.register
def on_message(event: fbchat.MessageEvent):
    # If you're not the author, echo
    if event.author.id != session.user.id:
        event.thread.send_text(event.message.text)

listener.run()

A few notes before we round this off:

  • I probably won't be adding caching, the requirements for this changes vastly across applications, so I'd rather provide a solid foundation that people can build this upon
  • See Version 2 wishlist #396 for the wishlist, and feature requests
  • If you want to see more examples of how the API has changed, you can look at the difference between the v1 examples and the current WIP examples

Thanks for your time 👍

@syspic
Copy link

syspic commented Feb 20, 2020

Oops sorry for getting to this so late. I do have some input

Interacting with threads looks a lot better now! But I still have one concern. Since all of this is done by ID and I don't want to send too many requests and get banned (I've gotten temporarily suspended more than I would like). My solution to this is making two dictionaries that maps a persons/group name to their/its id and backwards. So I can do something like this

name = idToName.get("10000")
id = nameToId.get("person")

Now you definitely have to make an object per group/person you're interacting with. Originally I could just do if not this id, skip the on_message or for every message I get in on_message, do this/send this. But now it seems like I would have to make a new group/user object in order to interact with it?

The listening seems pretty intuitive which is great! My guess is for the onX methods, youd just do more of

@listener.register
def on_X():

Is that right? If we have to do that, what is the default use? Originally, you could see what the Client's method was and then override it. Mostly it was just printing stuff out

Lastly, how difficult would it be to change from the v1 to the new v2? I assume that installing will be as easy as a pip install but the code would need a refactoring? I know this is a ways out though. Ive lost a few projects due to unfortunate events so I have to redo them . But I don't want to get too deep into a v1 project if I can wait for v2

@TheLastGimbus
Copy link
Contributor

TheLastGimbus commented Feb 21, 2020

But now it seems like I would have to make a new group/user object in order to interact with it?
and I don't want to send too many requests and get banned

This is important. Is User/Group already in fbchat.MessageEvent ?

@madsmtm
Copy link
Member Author

madsmtm commented Mar 4, 2020

I'll try to answer as best as I can, @syspic, but I may not have understood everything fully, so please elaborate if my answer doesn't make sense 😉.

My solution to this is making two dictionaries that maps a persons/group name to their/its id and backwards

A problem with nameToId is that you need to store the type of the thread as well - but that may not have been relevant in your use case.

Originally I could just do if not this id, skip the on_message or for every message I get in on_message, do this/send this

Like @TheLastGimbus noted, the fbchat.MessageEvent object you would be receiving would have a .thread attribute, that you could use, e.g. look up using event.thread.id.

youd just do more of

@listener.register
def on_X():

I haven't really thought this through entirely, but I think I wanted it to work with type hints. So the function name is irrelevant. The following two functions would both listen to the fbchat.MessageEvent, and would be called in an arbitary order:

@listener.register
def do_echo(event: fbchat.MessageEvent):
    # If you're not the author, echo
    if event.author.id != session.user.id:
        event.thread.send_text(event.message.text)

@listener.register
def kick_people(event: fbchat.MessageEvent):
    if event.message.text == 'Kick me!':
        event.thread.remove_participant(event.author)

Alternatively, if this seems a little magical, we could just take a parameter to the .register function:

@listener.register(fbchat.MessageEvent)
def do_echo(event):
    # If you're not the author, echo
    if event.author.id != session.user.id:
        event.thread.send_text(event.message.text)

how difficult would it be to change from the v1 to the new v2

Probably very. The API surface was almost entirely on the Client model, now it's split across several models.

Hope that answers your concerns, thanks for the questions!

@madsmtm
Copy link
Member Author

madsmtm commented Mar 4, 2020

To @TheLastGimbus, if you know the ID, creating a new User or Group object will always be "free", in the sense that it won't send a request to Facebook - And hence, you can't get banned for it 😉

@TheLastGimbus
Copy link
Contributor

Okay, so how do I know User.name by just passing it ID? How does it know? Does Group contain something like Group.members?

@madsmtm
Copy link
Member Author

madsmtm commented Mar 6, 2020

A little explanation:
There's User/Group, which you can create yourself and interract with:

user = fbchat.User(session=session, id="123")  # Does not call facebook, therefore doesn't contain further data
user.send_text('abc')
user.name  # Raises an error

And then there's UserData/GroupData (which inherits User/Group), which you receive from various functions, on which you can get the name, and interract with:

users = list(client.search_for_users('Abc', limit=1))  # Calls facebook, and recieves relevant data
print(users[0].name)
users[0].send_text('abc')

So when listening for messages, you would get a MessageEvent with a User object as it's thread. So you would have to make a dictionary yourself somewhere, that maps a user id to a name, e.g.:

id_to_name = {}
@listener.register(fbchat.MessageEvent)
def do_echo(event):
    if event.author.id not in id_to_name:
        # Fetch name, and cache for future usage
        author, = client.fetch_thread_info([event.author.id])
        id_to_name[author.id] = author.name
    if event.author.id != session.user.id:
        event.thread.send_text(f"Hello there {id_to_name[event.author.id]}")

@madsmtm
Copy link
Member Author

madsmtm commented Mar 6, 2020

I hope this makes sense, if you have ideas on how to improve this design, or if it seems confusing, let me know! 🙂

@MariposaLand
Copy link

MariposaLand commented Mar 6, 2020 via email

@madsmtm
Copy link
Member Author

madsmtm commented Mar 6, 2020

Hmm, I don't know what BERT is? But yeah, I do want to support the MARKETPLACE stuff, I just haven't had access to it myself, so I haven't seen the errors everyone else are getting 🙄

@MariposaLand
Copy link

MariposaLand commented Mar 6, 2020 via email

@MariposaLand
Copy link

MariposaLand commented Mar 6, 2020 via email

@madsmtm
Copy link
Member Author

madsmtm commented Mar 11, 2020

A quick notification, I've released an alpha version v2.0.0a1. The listening system is a bit different from what I wrote above, I wasn't entirely happy with it, so I ripped it out for now.

But now you have something to play with, then we can better start talking how it should work in detail!

@vincent-lg
Copy link

Hi everyone,

I've seen the gap between the current documentation and the code I have in my Git repository, so I investigated a bit more. I appreciate most of the changes I saw and thought I would just be installing using pip install -e waiting for it to go to production (it's not like what I'm coding is for production right away anyway).

One thing I couldn't work with, however, which is a big issue, is the listener. The on_... methods might have been error-prone but they were pretty simple to understand. I've seen two examples with the listener: one using, for what I can tell, a third-party library (blink?) which is out, if possible (no use getting more external libraries than strictly necessary in my opinion), the other one browsed ALL events and made a condition on event type which is a big no-no when you have more than just one event to react too.

Reading this post, I realize with pleasure I was highly mistaken and this listener is going to be great and have an interesting syntax. Registering functions sounds somewhat good to me. The only thing I fear: how to persist data in this context? I mean, if you do something like:

@listener.register(...)
def whatever():
   ...

... and if you need to store data in your callable, how are you going to do it? I don't much like using globals for that when I can help it.

LAST_USER = None

@listener.register(...)
def whatever():
   global LAST_USER # Haha gotcha!

Storing data as an instance attribute seems much more natural and Pythonic.

But it sounds like:

class Storage:

    @listener.register(...)
    def whatever(event):
        self.last_user = event.user

... might not fly since the callable hint types will be different (thanks to self as a positional argument). Am I missing something?

Also it seems the listener wasn't implemented in this way. I couldn't find an example of code, but if you do have one to share, I would appreciate.

Thanks for your great work.Hi everyone,

I've seen the gap between the current documentation and the code I have in my Git repository, so I investigated a bit more. I appreciate most of the changes I saw and thought I would just be installing using pip install -e waiting for it to go to production (it's not like what I'm coding is for production right away anyway).

One thing I couldn't work with, however, which is a big issue, is the listener. The on_... methods might have been error-prone but they were pretty simple to understand. I've seen two examples with the listener: one using, for what I can tell, a third-party library (blink?) which is out, if possible (no use getting more external libraries than strictly necessary in my opinion), the other one browsed ALL events and made a condition on event type which is a big no-no when you have more than just one event to react too.

Reading this post, I realize with pleasure I was highly mistaken and this listener is going to be great and have an interesting syntax. Registering functions sounds somewhat good to me. The only thing I fear: how to persist data in this context? I mean, if you do something like:

@listener.register(...)
def whatever():
   ...

... and if you need to store data in your callable, how are you going to do it? I don't much like using globals for that when I can help it.

LAST_USER = None

@listener.register(...)
def whatever():
   global LAST_USER # Haha gotcha!

Storing data as an instance attribute seems much more natural and Pythonic.

But it sounds like:

class Storage:

    @listener.register(...)
    def whatever(event):
        self.last_user = event.user

... might not fly since the callable hint types will be different (thanks to self as a positional argument). Am I missing something?

Also it seems the listener wasn't implemented in this way. I couldn't find an example of code, but if you do have one to share, I would appreciate.

Thanks for your great work.

@BahMike
Copy link

BahMike commented Apr 16, 2020

Does a configurable global rate limit sound like a good idea? It can be really basic like you set the time in seconds that you can send 10 requests in and if you attempt to make any more requests just add them to a queue that'll roll out the next 10 requests after the rate limit is over.

@TheLastGimbus
Copy link
Contributor

What you want to accomplish with it? Lower possibility to be caught as a bot by Facebook? I don't think it will help, and it will just add complexity. If you will keep sending 1 request every second, it's still gonna look suspicious. If you send 10 requests in 5 seconds but once for 2 days - it's okay.

@syspic
Copy link

syspic commented Jun 21, 2020

Hey! Its been a while since I've used this fbchat and want to use this again.

Last time, there was an issue where listening didnt work and it was fixed in the branch by tulir/fbchat-asyncio. Has that issue been fixed here/v1 and is v2 been released (I dont think it has)?

Thanks! Looking forward to getting into making some bots again. Mostly just listening and logging things

@TheLastGimbus
Copy link
Contributor

@syspic maybe try out the v2 alpha - it's been progressing for a while now so it should be somehow working now (?), and I'm sure @madsmtm will appreciate feedback on what needs to be changed

@syspic
Copy link

syspic commented Jul 1, 2020

@TheLastGimbus Thanks for the reply! (and sorry for the delay). I could try that out! Super dumb question, but how would I install it? Im assuming its not ready for pip install?

I mostly just want to listen (when a message comes in, reacts, etc) and log it to a file. Do you think that would be doable with v2 as it stands? I dont have plans of replying/changing chat name/etc as of now

Its super simple but v2 is different from v1 so ill have to get used to it lol

@TheLastGimbus
Copy link
Contributor

TheLastGimbus commented Jul 4, 2020

Honestly, I don't know, but v2 is probably capable of doing such things already.
To install it, try pip install fbchat==2.0.0a5

Or, as pypi site states, you can install it from source:

git clone https://github.com/carpedm20/fbchat.git
pip install fbchat

I need to get back to this library and see how it looks, and commit to it a bit, but I will most probably have time at the end of the super/after it ends :/

@syspic
Copy link

syspic commented Jul 19, 2020

quick update: I was able to get what I needed by using the V1 branch. I didn't want to go refactor everything. I think pip install fbhcat installs the v1 version. I never really looked into this but does anyone know how to tell what version the repo is at? If a new version, say 2.0.2 comes out, how could I tell?

@TheLastGimbus
Copy link
Contributor

TheLastGimbus commented Jul 19, 2020

pip install fbchat installs v1 version

Just add ==2.0.0a5 like I told you above

How to tell what version the repo is at?

As far as I know, v1 branch will always be at 1.9.x, or 1.x.x, and v2 development is going on master branch. Stage at which the branch is at certain version is marked with proper tag, named like v2.0.0a3

If a new version is out, how could I tell?

You can watch this repo on GitHub, select "Releases only" and you will get notification/email when new version comes out

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

6 participants