feat: Add --help, introduce "any" reason, explain filter when no notifications found
#196
Conversation
seveibar
changed the title
seveibar
changed the title
--help, introduce "any" reason, explain filter when no notifications found
There was a problem hiding this comment.
😭 indeed 😄. These changes all look appealing to me, but could you please split them up? I'm coincidentally about to post a blog post on doing that -JoshuaKGoldberg/dot-com#334 and the most relevant point here is that splitting things up means changelog & release generation plays more nicely. Also not blocking all of the stuff on requesting ~1 area to change.
--help: I have no requests for changes & think it's generally a good thing to do. 👍 ❤️logFilterWhenEmpty: I like the idea but think we should discuss the log format a bit. Not giving the same benefits to--watchusers is kind of unfortunate. Maybe there's a more generalized one-liner that could be printed for them each time? Maybe only the first log prints the full set of filter criteria? TBD?- If you don't have time to take this much more complicated, no worries - I can build off the changes here.
For each, I'd be happy to review a PR. If you really want to go the extra mile you could file an issue, but I don't mind filing them myself to save you the time. 😄
Regardless: thank you for sending this! Strong +1 from me on the direction and features. 🚀
| @@ -1,3 +1,4 @@ | |||
| coverage*/ | |||
| lib/ | |||
| node_modules/ | |||
| .aider* | |||
There was a problem hiding this comment.
| @@ -114,4 +114,27 @@ describe("pruneGitHubNotifications", () => { | |||
| ] | |||
| `); | |||
| }); | |||
|
|
|||
| it("logs a message when no notifications match the filters", async () => { | |||
There was a problem hiding this comment.
[Testing] If we're going with the feature as-is, I'd request a test for the inverse - that when logFilterWhenEmpty is false, it doesn't log. I.e.:
| it("logs a message when no notifications match the filters", async () => { | |
| it("does not log a message when no notifications match the filters and logFilterWhenEmpty is false", async () => { ... }); | |
| it("logs a message when no notifications match the filters and logFilterWhenEmpty is true", async () => { ... }); |
...but, TBD, maybe we'll go with something fancier?
There was a problem hiding this comment.
yea we should probably just do a verbose flag or something! I don't know how you feel about the debug module, but I'm also a big fan of that
JoshuaKGoldberg
added
the
status: waiting for author
PR Checklist
status: accepting prsOverview
Recently i started using this module again because I was missing notifications (email and slack bots just aren't good enough imo). I struggled to understand why it wasn't filtering anything for me and it turned out to be because I was a codeowner so the reason wasn't "subscribed" but actually "review_requested". I realized I didn't really care about the reason, if it's a dependency update I generally just don't want to be notified.
Anyway, this PR improves that debugging experience by printing the resolved filter configuration (with the defaults applied). It also adds a help menu and allows you to specify "any" for a reason.