Conversation
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 9531c1b in 58 seconds
More details
- Looked at
79lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. aw_notify/main.py:177
- Draft comment:
Consider handling specific exceptions likeFileNotFoundErrorforshutil.whichandsubprocess.CalledProcessErrorforsubprocess.runto provide more precise error handling and logging. - Reason this comment was not posted:
Confidence changes required:50%
The use ofshutil.whichis correct for checking the existence ofterminal-notifier. However, thesubprocess.runcall should handle potential exceptions more gracefully, especially considering the use ofcheck=True.
2. aw_notify/main.py:186
- Draft comment:
Be cautious withmsg.strip("-").replace("- ", ", ").replace("\n", ""). This could unintentionally alter the message content. Consider documenting this behavior or ensuring it aligns with expected message formats. - Reason this comment was not posted:
Confidence changes required:50%
Themsg.strip("-").replace("- ", ", ").replace("\n", "")line is modifying the message string. This could potentially alter the intended message content, especially if the message legitimately contains these characters.
Workflow ID: wflow_qE3L9DgDS6sERouQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
9531c1b to
11e6083
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 11e6083 in 35 seconds
More details
- Looked at
77lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. aw_notify/main.py:184
- Draft comment:
Unnecessary use ofstrip("-")inmsg.strip("-").replace("- ", ", ").replace("\n", ""). It could alter the message content unexpectedly. Consider removing it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Looking at the code, the strip("-") appears to be part of an intentional message formatting pipeline. The code is converting bullet points (like "- item1\n- item2") into a comma-separated list. The strip("-") removes any leading/trailing dashes, which makes sense as part of this transformation. Removing it could break the intended formatting.
I could be wrong about the intended message format. Maybe there are cases where stripping dashes could remove important content.
Looking at the code's usage (send_checkin function), messages are indeed formatted as bullet points with dashes. The strip("-") is a deliberate part of converting this format to a comma-separated list for terminal-notifier.
The comment should be deleted. The strip("-") appears to be an intentional part of the message formatting pipeline, converting bullet points to comma-separated lists.
Workflow ID: wflow_8Qb2BP3YqniyFnX8
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
11e6083 to
ec71acf
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ec71acf in 43 seconds
More details
- Looked at
83lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. aw_notify/main.py:186
- Draft comment:
Usingstrip("-")will only remove leading and trailing hyphens, not all hyphens. Consider usingreplace("-", "")instead to remove all hyphens from the message.
msg.replace("-", "").replace("- ", ", ").replace("\n", "")
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The current code appears intentional - it first strips leading/trailing hyphens, then replaces "- " with ", " to convert a bullet-point list into a comma-separated list. The comment's suggestion would remove ALL hyphens which could break hyphenated words. The current approach preserves hyphenated words while properly formatting bullet points.
Maybe there are edge cases where having leading/trailing hyphens is actually desired in the notification message? The current code could potentially remove meaningful hyphens at the start/end.
The code is formatting notification messages which are unlikely to need leading/trailing hyphens. The current approach sensibly preserves hyphenated words while cleaning up bullet-point formatting.
The comment should be deleted. The current code's handling of hyphens appears intentional and correct for converting bullet-point lists to comma-separated lists while preserving hyphenated words.
Workflow ID: wflow_GM3XRyLtJeOCEdaS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finally had enough of this not working on macOS.
Discovered terminal-notifier when looking for a CLI replacement to
notify-send, works well!Important
Adds
terminal-notifierfallback for notifications on macOS inaw_notify/main.py.notify_terminal_notifier()function inaw_notify/main.pyto send notifications usingterminal-notifieron macOS.notify()function to useterminal-notifieras a fallback on macOS whenDesktopNotifierfails.sys.platform == "darwin"to determine if running on macOS.This description was created by
for ec71acf. It will automatically update as commits are pushed.