-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: Add possibility to provide onSuccess function #53
Conversation
@BeyondEvil makes sense to me 👍 |
I like this concept! Could you add the example (or a similar one) to the docs? |
@@ -0,0 +1,37 @@ | |||
const chunkifyArray = (messageLines, maxLength, delimiter) => { |
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.
What does this do? Is this used somewhere or is it meant to be imported by our users?
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.
It's called here.
I figured the logic deserved it's own function. For readability, extendability, and usability. :)
lib/chunkifier.js
Outdated
let chunk = line + delimiter | ||
if ((message + chunk).length > maxLength) { | ||
resultLines.push(message.trimEnd()) | ||
message = chunk |
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.
what does this line do?
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 should probably explain the algorithm "inline". I'll add that when I add docs and tests.
But basically we have messageLines
which is an array of all the text split newline (\n
) and we have the maxLength
which is the size of each message chunk
So for each item in messageLines
we test if adding a new chunk
will put us over the limit (maxLength
). If it does we "reset" message
with this new chunk (line 10).
If it doesn't put us over, we keep concatenating the chunks to message
.
Hope that makes sense! 😂 @juliuscc
I take that as I should add docs and tests and turn this into a proper PR. 😊 |
c5123b6
to
f40134a
Compare
Some tests added. Will add more tests and docs tomorrow. |
Haha yes! I later saw that there wasn't any doc-updates 😁 I think the concept is great! 👍 |
That's it for tests. I'll try and add docs tomorrow. I named the test-files according to convention. Will rename the other test-files in a separate PR. |
6388a77
to
4529cb3
Compare
4529cb3
to
fbd419c
Compare
I'd say this is ready for review now @juliuscc @hannesrabo @AndrewLeedham 😊 |
|
||
If a function is provided with either the `onSuccessFunction` or `onFailFunction` options, it will be used for the respective slack message. The function should return an object that follows the [Slack API message structure](https://api.slack.com/docs/message-formatting). The function is passed two objects, `pluginConfig` and `context`, the same objects that are passed to [plugins](https://github.com/semantic-release/semantic-release/blob/master/docs/developer-guide/plugin.md#plugin-developer-guide). | ||
|
||
**Note**: This only works with a [configuration file](https://semantic-release.gitbook.io/semantic-release/usage/configuration#configuration-file) that exports an object (see below for an example). |
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.
Not sure if this is actually true? 🤔
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.
Hmm, I can not think of a way to make it work otherwise. Let's keep it like that unless someone corrects us 😁
@@ -21,7 +21,7 @@ Add the plugin to your npm-project: | |||
$ npm install semantic-release-slack-bot -D | |||
``` | |||
|
|||
The corresponding slack app has be installed in you slack workspace as well. Follow the instructions under [configuration](#configuration) for more information. | |||
The corresponding slack app has to be installed in your slack workspace as well. Follow the instructions under [configuration](#configuration) for more information. |
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.
Great that you have cleaned up in the Readme! I didn't know that there were that many spelling mistakes 😅
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.
This all looks great to me! 👍
Thanks for the hard work! @BeyondEvil 🥇 |
🎉 This PR is included in version 1.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This adds two new options:
onSuccessFunction
onFailFunction
Both functions takes
pluginConfig
andcontext
as arguments so that the user has access to all the Semantic Release configuration. They also take precedence over the respective template option (we can change this of course, I don't have a strong opinion either way).Before I add documentation and tests, I just want to make sure we're fine with this type of addition. @juliuscc @hannesrabo cc: @AndrewLeedham (it was your idea after all)
See also #39 and #26
Example config (tested, but with irrelevant parts removed)