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

Implemented more granular parameters for Telegram notifications #4874

Merged
merged 3 commits into from
Aug 29, 2016
Merged

Implemented more granular parameters for Telegram notifications #4874

merged 3 commits into from
Aug 29, 2016

Conversation

DBa2016
Copy link
Contributor

@DBa2016 DBa2016 commented Aug 28, 2016

Short Description:

"alert_catch" now can be either a list as before (alert on every catch of the corresponding pokemon) or a dictionary of dictionaries, of the form:
"alert_catch": {
"PokemonName": { "operator": "and/or", "iv": 0.9, "cp": 1000}
}

@mention-bot
Copy link

@DBa2016, thanks for your PR! By analyzing the annotation information on this pull request, we identified @askovpen, @bruno-kenji and @mjmadsen to be potential reviewers

@Gobberwart
Copy link
Contributor

You know, I quite like this concept, but its association with Telegram puts me off. I really don't want (and won't use) yet another messaging application.

Can I suggest you think about separating the various information-generating code from the actual message-sending code. That way, the information-generating stuff could be re-used to send notifications via other protocols, ie. email, skype, messenger etc.

Eg.

delivery_system = telegram/skype/whatever (obviously needs supporting code)
event = level_up/catch_pokemon/awarded_badge etc.
msg = nicely formatted notification message

then..

delivery_system.send_message(msg)

Let the delivery_system code worry about how to delivery the message, while the rest just deals with handling events and passing them on.

Just my 2 cents. Hope it makes sense.

@DBa2016
Copy link
Contributor Author

DBa2016 commented Aug 29, 2016

@Gobberwart : the original Telegram integration does not originate from me, I only adjusted the notification config and some other things.

In principle, what you are suggesting makes sense, and it would involve the following:

  1. define an interface and create an abstraction layer which the actual "information-generating" code will be using
  2. create an implementation for every needed IM system, starting with Telegram (because it is already there and should not be removed without a good reason).

None of these two parts is depending nor anyhow related to this PR. You are welcome to go ahead and implement your wish - I will very unlikely do because I am okay with Telegram and don't see a need for further IMs. :)

@Gobberwart
Copy link
Contributor

@DBa2016 No problem, just thought I'd comment while I had the opportunity. Never know who's watching. Not something I'm planning to do anything about immediately, but if anyone wants to run with it, go nuts :)

@solderzzc
Copy link
Contributor

@Gobberwart The master setting is whom you can send for. Please check the code inside and correct me if I'm wrong.

@Gobberwart
Copy link
Contributor

@solderzzc The master setting? Sorry, not following you.

@solderzzc
Copy link
Contributor

@Gobberwart this one:
"config": {
"enabled": false,
"master": null,

You can set the master to your own id, or the message will not send out.

@Gobberwart
Copy link
Contributor

Gobberwart commented Aug 29, 2016

@solderzzc Not my point. I'd like to see alert messages able to be sent from whatever messaging system you choose.. as in...

"alerts": {
  "config": {
    "enabled": true,
    "delivery_system": email
  }
}

and then...

"email": {
  "config": {
    "smtp_host": "smtp.blahblah.com",
    "username": "blahblah",
    "password": "halbhalb"
  }
}

etc.

Sure I can set the Telegram id to whatever I want, but I am not interested in Telegram. I am, however, interested in an alert delivery system.

@solderzzc
Copy link
Contributor

@Gobberwart sure, get it.

trigger = self.pokemons["all"]
else:
return
if (not "operator" in trigger or trigger["operator"] == "and") and data["cp"] >= trigger["cp"] and data["iv"] >= trigger["iv"] or ("operator" in trigger and trigger["operator"] == "or" and (data["cp"] >= trigger["cp"] or data["iv"] >= trigger["iv"])):
Copy link
Contributor

Choose a reason for hiding this comment

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

There's good logic operator code in the pokemon catch worker, would you like to use that code @DBa2016

@solderzzc
Copy link
Contributor

LOGIC_TO_FUNCTION = { 'or': lambda x, y, z: x or y or z, 'and': lambda x, y, z: x and y and z, 'orand': lambda x, y, z: x or y and z, 'andor': lambda x, y, z: x and y or z }

@Gobberwart
Copy link
Contributor

Does this work with "any" or "all"?

@solderzzc
Copy link
Contributor

Seems the all is included.
elif "all" in self.pokemons:

  •                    trigger = self.pokemons["all"]
    
  •                else:
    

@Gobberwart
Copy link
Contributor

Cheers mate, I was too lazy to look :)

@solderzzc
Copy link
Contributor

Let's have it merge, leave the code fine tune later.

@solderzzc
Copy link
Contributor

@Gobberwart Do you want to try the pull approval ?

@Gobberwart
Copy link
Contributor

@solderzzc Approved... do I just click "Merge pull request" now?

@solderzzc
Copy link
Contributor

yes, I think so...

@Gobberwart Gobberwart merged commit a388c10 into PokemonGoF:dev Aug 29, 2016
@Gobberwart
Copy link
Contributor

Yay! :)

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.

4 participants