Skip to content
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

[v2] Add documentation for async middleware and App#processEvent #381

Merged
merged 5 commits into from
Mar 19, 2020

Conversation

barlock
Copy link
Contributor

@barlock barlock commented Jan 24, 2020

Summary

Adds documentation for code in PR: #375 and #380

Depends on #369

Important notes

My Japanese is bad. Perhaps non-existant? Honestly the only language I know how to speak is ecmascript 🤷‍♂.

Should someone make a PR to my PR with JP edits? I'll try and notate where they need to happen.

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Jan 24, 2020

Codecov Report

Merging #381 into @slack/bolt@next will increase coverage by 2.03%.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           @slack/bolt@next    #381      +/-   ##
===================================================
+ Coverage             83.56%   85.6%   +2.03%     
===================================================
  Files                     7       7              
  Lines                   505     514       +9     
  Branches                145     149       +4     
===================================================
+ Hits                    422     440      +18     
+ Misses                   54      50       -4     
+ Partials                 29      24       -5
Impacted Files Coverage Δ
src/ExpressReceiver.ts 71% <0%> (-1.39%) ⬇️
src/middleware/builtin.ts 82.2% <0%> (-0.2%) ⬇️
src/App.ts 90.25% <0%> (+2.9%) ⬆️
src/middleware/process.ts 100% <0%> (+12.5%) ⬆️
src/errors.ts 93.93% <0%> (+12.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5881471...95cb527. Read the comment docs.

@barlock barlock changed the title Promise docs Add documentation for async middleware and App#processEvent Jan 24, 2020
@seratch
Copy link
Member

seratch commented Jan 24, 2020

@barlock
Thanks for your attention to the documents! Also, as a native Japanese speaker, I really appreciate your efforts on Japanese ones as well and I don't see any issues with the changes so far. So, regarding this PR, we can go with your changes as-is.

Should someone make a PR to my PR with JP edits? I'll try and notate where they need to happen.

If a PR requires updates to not only code snippets but sentences (like this), I usually work on it. 💪

@barlock barlock changed the base branch from master to @slack/bolt@next February 3, 2020 17:31
@barlock barlock requested a review from aoberoi February 3, 2020 17:31
@barlock
Copy link
Contributor Author

barlock commented Feb 3, 2020

Updated to be based on @slack/bolt@next and updated the commits. This is ready to be reviewed/merged now.

@seratch
Copy link
Member

seratch commented Feb 4, 2020

@aoberoi This PR even including the Japanese ones looks great to me. I think we can merge this one now.
@shaydewael I know you're busy but if you have a chance to take a look at this, that'll be really helpful as this one is about the docs. 🙏

@seratch seratch changed the title Add documentation for async middleware and App#processEvent [v2] Add documentation for async middleware and App#processEvent Feb 17, 2020
Copy link
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just some small feedback

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -40,6 +40,10 @@ class simpleReceiver extends EventEmitter {
this.app.post(endpoint, this.requestHandler.bind(this));
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 31-33 still extends EventEmitter

README.md Outdated Show resolved Hide resolved
@stevengill stevengill merged commit 3a1fc9e into slackapi:@slack/bolt@next Mar 19, 2020
Copy link
Contributor

@aoberoi aoberoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad this has already merged, but I just wanted to take some notes regarding a few things I noticed. I will likely follow up with another PR to address these comments.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
docs/_advanced/middleware_global.md Show resolved Hide resolved
docs/_advanced/receiver.md Show resolved Hide resolved
docs/_advanced/receiver.md Show resolved Hide resolved
docs/_advanced/receiver.md Show resolved Hide resolved
docs/_advanced/receiver.md Show resolved Hide resolved
stevengill added a commit to stevengill/bolt that referenced this pull request Mar 24, 2020
@stevengill stevengill mentioned this pull request Mar 24, 2020
2 tasks
stevengill added a commit to stevengill/bolt that referenced this pull request Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants