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

Add 2 methods: fetch all threads, fetch all users involved in threads #300

Merged
merged 13 commits into from
Jan 31, 2019

Conversation

2FWAH
Copy link
Contributor

@2FWAH 2FWAH commented Jun 7, 2018

As fetchAllUsers() seems broken (#282 ), I added two methods:

  • fetchThreads(thread_location) to fetch all threads in ThreadLocation
  • fetchAllUsersFromThreads(threads) to fetch all users involved in threads.

Example: to fetch all users in INBOX simply use:
Users = client.fetchAllUsersFromThreads(client.fetchThreads(ThreadLocation.INBOX))

fetchAllUsersFromThreads() might be long to finish, should progress be printed? (based on the number of threads given as parameter)

2FWAH added 5 commits June 1, 2018 22:59
Add a method to get all threads in Location (INBOX, ARCHIVED...)
Use "self" instead of "client"
Add a method to get all users involved in threads (given as a parameter)
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Thanks for you submission! I like your fetchThreads implementation, it's a nice abstraction, but perhaps it would be nice to have a limit parameter, so that you don't fetch a lot of threads you really don't need?

fbchat/client.py Outdated Show resolved Hide resolved
fbchat/client.py Outdated Show resolved Hide resolved
fbchat/client.py Outdated Show resolved Hide resolved
fbchat/client.py Outdated Show resolved Hide resolved
@2FWAH
Copy link
Contributor Author

2FWAH commented Jun 12, 2018

it would be nice to have a limit parameter

Yes, BTW an after parameter would be useful to fetch threads after a timestamp.
I think by default no limit or after setting should be set, right?

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I was thinking, merging fetchThreadList into fetchThreads altogether (or the other way around?) could be a cool solution.
That is, make is such that we have parameters before, after and limit (and maybe offset?), and then we'd able to combine these in any way we like.
For example, after=1514761200 and before=1519858800 would fetch threads from the start of January to start of March. Then you could add a limit=3, in case you only needed three threads.
Also, you could implement after and before as datetime objects, see #278

fbchat/client.py Outdated Show resolved Hide resolved
fbchat/client.py Outdated Show resolved Hide resolved
fbchat/client.py Outdated Show resolved Hide resolved
@2FWAH
Copy link
Contributor Author

2FWAH commented Sep 16, 2018

Totally forgot about this PR! 👎
Still relevant today with the refactoring of the API (aka V2.0.0)?

@madsmtm
Copy link
Member

madsmtm commented Sep 17, 2018

I'd say it's still relevant, v2.0.0 is still quite far away ;)

@madsmtm
Copy link
Member

madsmtm commented Sep 17, 2018

And besides, there's nothing preventing us from using part of the implementation from here in the new API

@2FWAH
Copy link
Contributor Author

2FWAH commented Sep 21, 2018

Done, testing needed :)
Support for datetime objects for before and after parameters is not yet implemented.
Should be part of #278 no?

By the way is it possible to know last modification date of some threads present in the test accounts?
That would be useful to add tests for before and after parameters.

fbchat/client.py Outdated Show resolved Hide resolved
if before is not None or after is not None:
for t in list(threads):
last_message_timestamp = int(t.last_message_timestamp)
if (before is not None and last_message_timestamp > before) or \
Copy link
Member

Choose a reason for hiding this comment

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

It seems kinda unclean that this is duplicated further up, is there a way we can refactor, and merge these?

@madsmtm
Copy link
Member

madsmtm commented Sep 27, 2018

You're right about not using datetime objects yet, at least that'll give some sort of consistency ;)

But no, I can't think of a way to properly test these methods

@madsmtm madsmtm merged commit 7f0da01 into fbchat-dev:master Jan 31, 2019
@madsmtm
Copy link
Member

madsmtm commented Jan 31, 2019

I went ahead and did some of the changes I requested, since this PR would have gotten lost otherwise (and I really wanted this functionality 😁). Thanks for your work! 🎉

@chelsybradley15 chelsybradley15 linked an issue Mar 25, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants