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 functionality to support Pushbullet Channels #18

Closed
mikebirdgeneau opened this issue Dec 29, 2014 · 14 comments
Closed

Add functionality to support Pushbullet Channels #18

mikebirdgeneau opened this issue Dec 29, 2014 · 14 comments

Comments

@mikebirdgeneau
Copy link
Contributor

Refer to 57ebf50 for an update to documentation describing how to create channels for Pushbullet, and a brief reference to the Pushbullet API and the target parameter channel_tag which allows the target channel to be specified for a push.

@mikebirdgeneau
Copy link
Contributor Author

Here are some suggested steps in order to implement this:

  1. Need to determine parameter to be passed to pbPost, in the earlier changes I made, I used the parameter channel.
  2. The second step is determining how the logic should work when channel is specified. For example, should recipients / email be ignored? Or should channel be ignored if recipients / email specified? Or should all destinations for the post be used? I don't have a strong preference as to how this is handled, but deciding this before making changes will make this much easier.
  3. Add channel_tag to the curl command.
  4. Tests should be added to test the channel functionality.

@eddelbuettel
Copy link
Owner

Good points. In order:

  1. I think channel makes sense, else it may be hard to disambiguate messaging and emailing modes.
  2. That one is important. I am a bit afraid of creating an impasse between the 2 x 2 x 2 states. On the other hand we could just that messaging > email > channel. Thoughts?
  3. Yup.
  4. Yes, we'd feel much better about the changes once we see them tested...

I'll get to work on your other submissions in a bit.

@eddelbuettel
Copy link
Owner

Hi @mikebirdgeneau -- Happy New year!

I guess the holidays/new year got in the way. Shall we resume testing this?

@mikebirdgeneau
Copy link
Contributor Author

Hi @eddelbuettel - Happy new year to you as well. I found myself quite busy with my day job!
Anyhow, I've taken a quick stab at the changes in order above, using your suggestion of messaging > email > channel.

For testing, we may be able to use the JSON results to develop logic to ensure that tests pass as intended. Do you have any good examples of where you perform tests for another package that might be a good template to follow?

All changes are in my fork here:
mikebirdgeneau/rpushbullet@eddelbuettel:master...master

I'll create a pull request to review and discuss the changes.

@eddelbuettel
Copy link
Owner

Thanks so much -- looks really promising so far.

@eddelbuettel
Copy link
Owner

I added some support in R/init.R. Now we can have test email and channel targets.

However, it looks like broke/lost some functionality. Very first test:

R> pbPost("note", "A Simple Test", "We think this should work.\nWe really do.")
Error in FUN(""[[1L]], ...) : 
  argument "email" is missing, with no default
R> 

@mikebirdgeneau
Copy link
Contributor Author

I've updated the tests to use those variables you've added and submitted a pull request (#21).
Everything seems to run successfully for me now, so if your problem persists we can debug.

@eddelbuettel
Copy link
Owner

Awesome. I spent most of yesterday with Rcpp* so I did not get to it.

I think I'll rewrite tests/simpleTests.R as a caller to 'normal' unit tests, that way we can spread out the combinations of type and recipient. OK if I use RUnit which has been reliable for me in the past?

@mikebirdgeneau
Copy link
Contributor Author

I've never used RUnit, so I'm happy to follow your lead on this!
Aside: just received a push from you.

@eddelbuettel
Copy link
Owner

Yes, sorry -- just replaced your remaining test email and channel settings.

I think I fixed why the first call in the test script failed, we tested email for NA but had set it to ''. Doh.

@eddelbuettel
Copy link
Owner

Just committed -- give it a whirl when you have moment.

@eddelbuettel
Copy link
Owner

Ok, I no longer think we have to go to RUnit. So no rush there.

But you had made some comments days ago about a README (or Rd file?) needing to change once we have channels. As channels now work, a) what is that edit, and b) shall we make it?

@mikebirdgeneau
Copy link
Contributor Author

Everything seems to be working for me at the moment.
Readme.md changes are just updating to say that channels can be used in the package. I've made the change in this commit:
mikebirdgeneau@7614a7d

@eddelbuettel
Copy link
Owner

Ok, great -- as this wasn't a PR I just took it by hand. Will do the same with technically open PR and then close it, ok?

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

No branches or pull requests

2 participants