Skip to content
This repository has been archived by the owner on Jul 9, 2022. It is now read-only.

Added getAllIDs like getUserID but not limited to users(depending on arg), re-wrote getUserID to call it #459

Conversation

mitchcapper
Copy link
Contributor

Mainly helpful for getting the ID's for bots to message.

@ravkr
Copy link
Contributor

ravkr commented Apr 9, 2017

It doesn't look good to have 2 different functions calling the same url (/ajax/typeahead/search.php) but with changed parameters
Maybe we should have something like api.getFBID() or api.getSearchResut() or maybe just api.search()
first parameter would be query you want to search and second could be optionswhere you can specify if you want users, fanpages, groups etc.
@Schmavery what do you think?

@mitchcapper
Copy link
Contributor Author

@ravkr I completely agree. Honestly I would have preferred to just overload the current function to take another parameter, however given the current callback structure just adding a param is not the easiest. Now technically it could be done, technically as the user HAS to specify a callback we can assume if called with 2 args it is the old function and if called with 3 then it is the new signature. New signature could be:
getUserID(name,allowNonUsers,callback).

Happy to update the PR with whatever the consensus is. Mainly wanted to avoid any breaking changes.

@Schmavery
Copy link
Owner

@mitchcapper for what it's worth, this "optional argument" pattern is used for many other api functions. Is there also the option of simply always including everything?

@mitchcapper
Copy link
Contributor Author

@Schmavery are you ok with the overload method I describe? Always including everything would be a behavior change, but as the old way would throw an error I doubt it would be that big of a deal. Let me know which direction you want to go and I will update the PR.

@Schmavery
Copy link
Owner

I'm wondering if technically it's not really a behaviour change. Technically at any point there could be new hits for any search result, so adding some new results (due to including everything) wouldn't affect the current behaviour as far as I can tell. If you agree with this, then we're probably best to not add the option to keep it simple.

@mitchcapper
Copy link
Contributor Author

Closing in favor of PR #462

@Schmavery
Copy link
Owner

Ok, for future reference you can update PRs by pushing to the fork you used for the PR 😄

@mitchcapper
Copy link
Contributor Author

@Schmavery thanks, aware of that, it was more just the fact the title/branch name no longer were really represented what the PR did so I figured a new one:)

@mitchcapper mitchcapper deleted the allow_getting_ids_for_non_users_pr branch April 10, 2017 19:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants