search

RayPS / node-red-contrib-pushover

A Node-RED node to send Pushover notification
Other
17 stars 6 forks source link

a pushover node already exists #1

Open cflurin opened 6 years ago

cflurin commented 6 years ago

Have look at node-red-node-pushover

RayPS commented 6 years ago

There is a different. This node supports image attachment in notification which is node-red-node-pushover doesn't. I also make this option support both local file path and remote url path which will make this easy to use and flexible. You must try this one.

Paul-Reed commented 6 years ago

So why didn't you propose these changes to the author of node-red-node-pushover instead of creating a node with similar functionality?

RayPS commented 6 years ago

Because the change will break the existing flows, not just the options, this node included a pushover-keys config node to store credentials which is necessary.

Paul-Reed commented 6 years ago

Have you approached the authors and discussed whether this could be integrated (Slack or forum). Breaking changes are not good, but sometimes necessary to ensure that timely development can progress.

cymplecy commented 6 years ago

If I could add my 2 cents - calling it the same name as the core node caused me a lot of confusion.

Took me some while to work out that this was a new node and not an update to the existing core node

Although it can be more work - discussing it on the forum and then issuing a pull-request is the best way to help out all Node-RED users.

If the core devs reject your enhancements, then make a contrib node BUT make it obvious (by changing last part of the name) that it is NOT the core node

RayPS commented 6 years ago

@cymplecy The core pushover node is not installed in my Node-Red When i was developing this node, I just realized this problem, I didn't test with both of them installed. Sorry for that!

Edit: this node is renamed to pushover api, this problem will not happened again and users can have both installed.