-
Notifications
You must be signed in to change notification settings - Fork 4
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
Support Bark #19
base: main
Are you sure you want to change the base?
Support Bark #19
Conversation
Hi, I am currently working on improving the code quality. So the PR would be reviewed later. Meanwhile, you are supposed to keep syncing with the latest main branch. (Mainly for performing checks.) Thanks for contributing. |
OK, that's great, improving code quality is not easy, thanks for your excellent work ! Should I close this PR and re-pull it ? |
Actually you just need to run As for keeping or closing, it depends on you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution.
I have commented some problems with code quality.
Meanwhile, I think it's better to leverage the benefits of bark's multi-levels notification support.
Currently the logic is totally the same as real-time message APP like DingTalk, wasting the flexibility of the framework, resulting code redundancy.
For bark, I think it can be better to send passive message for exp start, active message for exp finished, timeSensitive/active for error message.
I am not familiar with bark currently, but I think using the plain logic wastes a lot.
# Setup. | ||
self.cfg = cfg | ||
self.device_token = cfg['device_token'] | ||
self.url = 'https://api.day.app/' + self.device_token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad (ambiguous) variable names.
You have 'url' in configuration setting, but you also have self.url
which is generated with device_token
, the two 'url' are not the same things. You need to assign better variable names.
self.icon = '' # TODO: add icon support. | ||
|
||
def _gen_sign(self, secret): | ||
timestamp = int(datetime.now().timestamp()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useless function. Plz remove.
smtp_port: <?> | ||
sender_email: sender_email=<?> # [email protected] | ||
sender_pwd: sender_pwd=<?> | ||
receiver_email: receiver_email=<?> # [email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pay attention to this. I have fixed the problem this time.
|
||
def format_information(self) -> dict: | ||
information = { | ||
'title': f'{self.readable_time} @ {self.host}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the best choice for tittle
.
def format_information(self) -> dict: | ||
# Never send empty paragraph, it would be ugly. | ||
information = { | ||
'title': f'{self.readable_time} @ {self.host}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the best tittle.
Add Bark Backends and docs