Skip to content

Remove global token and init()#27

Closed
amelchio wants to merge 1 commit intoThibauth:masterfrom
amelchio:local-token
Closed

Remove global token and init()#27
amelchio wants to merge 1 commit intoThibauth:masterfrom
amelchio:local-token

Conversation

@amelchio
Copy link
Copy Markdown

@amelchio amelchio commented Jul 28, 2018

This removes the global token and instead uses a per-Client value, allowing access to multiple registered applications by initializing several Client objects.

The issue was reported in home-assistant/core#11641

@jwater7
Copy link
Copy Markdown

jwater7 commented Jul 28, 2018

@Thibauth - can you take a look at this pr and see if a new release can be made with it included? There’s a few of us waiting on a fix to get pushover working again in Home Assistant - thanks!

@Thibauth
Copy link
Copy Markdown
Owner

Thanks a lot for the fix @amelchio ! The use of a global api_token was indeed ugly (not to mention bug-inducing in contexts where one want to use multiple tokens) and something I wanted to fix eventually. Your PR looks good to me and I would be happy to merge it and create a new release.

However, I now wonder whether a better way to structure this library would be to have a Pushover class instantiated with the API token. This class would have a send method taking as argument the user key. So the flow would look like this:

po = Pushover(api_token)
po.send(user_key, "Hello world!")

I would be curious to hear your opinion about this. If we believe this is better, then I would prefer to make this change before a new release so as to minimize the number of backward incompatible changes.

@amelchio
Copy link
Copy Markdown
Author

amelchio commented Aug 1, 2018

I see where you are coming from. For the Home Assistant case, though, we have just a single user_key and having it in the constructor is fine.

I don't have much of an opinion either way. I only passed by trying to fix this bug :)

@jwater7
Copy link
Copy Markdown

jwater7 commented Aug 14, 2018

Any movement on this? - to me, it looks good to merge so that the bugs are fixed, then we can focus on ways to improve?

@amelchio
Copy link
Copy Markdown
Author

At this point, I agree with @jwater7 :)

@amelchio
Copy link
Copy Markdown
Author

@Thibauth what's the status on this one? :-)

@Thibauth
Copy link
Copy Markdown
Owner

Sorry for the long period of inactivity, I just started working back on this. I ended up going for the bigger refactoring that I described above. This breaks backward compatibility but I think it is better in the long run. If you have a chance, it would be great if you could test to make sure everything is alright. I also need to update the documentation to reflect the changes and make a new release.

@amelchio
Copy link
Copy Markdown
Author

amelchio commented Nov 8, 2018

I adjusted to your changes and it works fine 👍. So please go ahead and make a new release :-)

@amelchio
Copy link
Copy Markdown
Author

Do you have an ETA for this?

@asyba
Copy link
Copy Markdown

asyba commented Jan 2, 2019

any news about this??

@ashmckenzie
Copy link
Copy Markdown

@Thibauth is there any news on your refactor? Are we able to assist at all?

@amelchio
Copy link
Copy Markdown
Author

@Thibauth If you have not planned more changes, could you please do a PyPI release?

@amelchio
Copy link
Copy Markdown
Author

It has been a year now. At this point I consider the project abandoned so I am going to close this PR.

@amelchio amelchio closed this Jul 29, 2019
@jwater7
Copy link
Copy Markdown

jwater7 commented Dec 24, 2019

NOTICE: Now that Apprise is available (https://www.home-assistant.io/integrations/apprise/) (https://github.com/caronc/apprise/) I've migrated my automatons to use the new apprise notify service (using the pushover/pover URL) and I no longer have this issue.

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.

5 participants