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.

Sign up for GitHub

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

Jump to bottom

Implement modularized notification system #61

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Implement modularized notification system #61

wants to merge 6 commits into from

Conversation

moritzketzer
Copy link
Contributor

  • Abstracted notification specifics out of runSimulation.
  • Added notifier parameter to runSimulation to use specific notifiers.
  • Added support for multiple notifiers to be used simultaneously.
  • Moved notification handling to notifications.R.
  • Added Notifier S3 class to represent notifiers.
  • Added TelegramNotifier and PushbulletNotifier S3 classes.
  • Added new_TelegramNotifier and new_PushbulletNotifier constructors.
  • Updated documentation to reflect changes in notification handling.

@moritzketzer
Implement modularized notification system
40e4844 
- Abstracted notification specifics out of `runSimulation`.
- Added `notifier` parameter to `runSimulation` to use specific notifiers.
- Added support for multiple notifiers to be used simultaneously.
- Moved notification handling to `notifications.R`.
- Added `Notifier` S3 class to represent notifiers.
- Added `TelegramNotifier` and `PushbulletNotifier` S3 classes.
- Added `new_TelegramNotifier` and `new_PushbulletNotifier` constructors.
- Updated documentation to reflect changes in notification handling.
@moritzketzer
Copy link
Contributor Author

@philchalmers since I dont use pushbullet it would be nice if you could test if it works :)

@moritzketzer
Copy link
Contributor Author

@philchalmers did you intend that one may also pass notification = c("condition", "complete")?
because this currently does not work and as I read it a character vector might imply that this is possible.
maybe change in the runsimulation docs from "an optional character vector input" to "an optional string".

@philchalmers
Copy link
Owner

This looks really nice, thanks for the contribution! Looks nicer than my previous pushbullet approach (I don't use that much anyway as larger simulation typically get submitted to SLURM clusters, but this is nice nevertheless).

Could you a) remove the devtools::load_all() from the test file, and b) add yourself as a contributor to the DESCRIPTION file? Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philchalmers philchalmers philchalmers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@moritzketzer @philchalmers