refactor: move slack url creation to its own package#5225
Conversation
Signed-off-by: Santiago Fernández Núñez <[email protected]>
Signed-off-by: Santiago Fernández Núñez <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SoloJacobs
left a comment
There was a problem hiding this comment.
It looks like this was largely AI-generated. We'd ask that contributions be reviewed and understood by the author before submitting, including making sure the commit message accurately describes what the code actually does. The message here says "refactor" but the change alters behavior. Happy to look at it again once it's been properly reviewed on your end.
hi, @SoloJacobs , sorry for the delay. one comment before all: this is still a draft, that's why i didn't share it on slack. regarding the comments, i not only reviewed but wrote code, the PR was not solely generated by AI. i chose a "refactor" label because the current behaviour was not change (the same URLs are generated in the current flow). what the PR did add was the ability of creating URLs for other methods... but they're not in use nowadays. anyways, it might be worth using a "feature" if you believe that potential use warrants it. |
|
Thanks for the context. To be more precise though: The commit description says "move", which implies pure relocation.
So, the "refactor" part is not really the issue, you could leave it. Although, |
Part 2 of a series implementing threaded message support for the Slack notifier. Full picture: #5150. Depends on #5224.
Summary
Extracts Slack API URL resolution logic from
slack.gointo a new internal packagenotify/slack/internal/apiurl. Adds comprehensive unit tests covering all resolution paths (direct URL, file-based, global config).