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.

Sign up for GitHub

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

Jump to bottom

Adding deferInitialization docs. Fixes #1304. #1308

Merged
merged 2 commits into from
Feb 23, 2022
Merged

Adding deferInitialization docs. Fixes #1304. #1308

merged 2 commits into from
Feb 23, 2022

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Feb 8, 2022 •
edited
Loading

I will definitely need a review on the Japanese docs since I just ran the text through Google Translate 😅

This PR fixes #1304.

@filmaj filmaj added the docs M-T: Documentation work only label Feb 8, 2022
@filmaj filmaj added this to the 3.10.0 milestone Feb 8, 2022
@filmaj filmaj self-assigned this Feb 8, 2022
@filmaj filmaj mentioned this pull request Feb 8, 2022
Closed
@codecov
Copy link

codecov bot commented Feb 8, 2022 •
edited
Loading

Codecov Report

Merging #1308 (f6fc378) into main (362621e) will increase coverage by 0.48%.
The diff coverage is n/a.

❗ Current head f6fc378 differs from pull request most recent head bdbb5e5. Consider uploading reports for the commit bdbb5e5 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1308      +/-   ##
==========================================
+ Coverage   72.76%   73.24%   +0.48%     
==========================================
  Files          17       17              
  Lines        1465     1439      -26     
  Branches      436      431       -5     
==========================================
- Hits         1066     1054      -12     
+ Misses        310      300      -10     
+ Partials       89       85       -4
Impacted Files Coverage Δ
src/receivers/SocketModeReceiver.ts 88.88% <0.00%> (+0.12%) ⬆️
src/receivers/HTTPReceiver.ts 51.06% <0.00%> (+0.20%) ⬆️
src/WorkflowStep.ts 90.69% <0.00%> (+0.22%) ⬆️
src/receivers/ExpressReceiver.ts 60.97% <0.00%> (+0.27%) ⬆️
src/App.ts 85.05% <0.00%> (+2.35%) ⬆️

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 362621e...bdbb5e5. Read the comment docs.

srajiang
srajiang approved these changes Feb 8, 2022
View reviewed changes
Copy link
Contributor

@srajiang srajiang left a comment

Choose a reason for hiding this comment

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

👍 Englais looks good to me. Very minor comments!

docs/_advanced/deferring_initialization.md Outdated Show resolved Hide resolved
docs/_advanced/deferring_initialization.md Outdated Show resolved Hide resolved
seratch
seratch requested changes Feb 8, 2022
View reviewed changes
docs/_advanced/deferring_initialization.md Outdated

// Must call init() before start() within an async function
(async () => {
await app.init();
Copy link
Member

@seratch seratch Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you add try/catch clause to synchronously catch he error and terminate the process? https://github.com/slackapi/bolt-js/pull/1303/files#r799876413 the exist status should not be necessarily 255, though. process.exit(1) would be fine too.

docs/_advanced/ja_deferring_initialization.md Outdated
@@ -0,0 +1,30 @@
---
title: 初期化の延期
Copy link
Member

Choose a reason for hiding this comment

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

It's already understandable! 🤣 but @wongjas and I can suggest updates once we finalize the English version

@filmaj filmaj requested review from seratch and wongjas February 9, 2022 14:11
@filmaj
Copy link
Contributor Author

filmaj commented Feb 9, 2022

Thanks for the suggestions! I've added them and updated the PR. If there are no other suggestions for the English docs, then perhaps the Japanese docs could get a pass to try to go from "understandable" to "good" 😄 ?

@seratch
Copy link
Member

seratch commented Feb 9, 2022

@filmaj Thanks for revising the doc quickly! @wongjas will come up with JP translation parts today (I've already reviewed most of his translation)

@wongjas
Copy link
Member

wongjas commented Feb 10, 2022

Added the Japanese translation suggestions. One final check please @seratch! 🙇

@seratch
Copy link
Member

seratch commented Feb 10, 2022

@wongjas I might forget submitting your review - I cannot see your suggestions

wongjas
wongjas reviewed Feb 10, 2022
View reviewed changes
docs/_advanced/ja_deferring_initialization.md Outdated Show resolved Hide resolved
docs/_advanced/ja_deferring_initialization.md Outdated Show resolved Hide resolved
docs/_advanced/ja_deferring_initialization.md Outdated Show resolved Hide resolved
docs/_advanced/ja_deferring_initialization.md Outdated Show resolved Hide resolved
docs/_advanced/ja_deferring_initialization.md Outdated Show resolved Hide resolved
docs/_advanced/ja_deferring_initialization.md Outdated Show resolved Hide resolved
docs/_tutorials/ja_reference.md Outdated Show resolved Hide resolved
seratch
seratch reviewed Feb 10, 2022
View reviewed changes
Copy link
Member

@seratch seratch left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@wongjas one correction (it was my fault, sorry)

docs/_advanced/ja_deferring_initialization.md Outdated Show resolved Hide resolved
@filmaj filmaj requested a review from seratch February 10, 2022 13:28
@filmaj
Copy link
Contributor Author

filmaj commented Feb 10, 2022

Thanks for the suggestions! I've added and squashed them. @seratch or @wongjas, feel free to merge if you think the Japanese documentation is now ready 👍

seratch
seratch approved these changes Feb 10, 2022
View reviewed changes
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

LGTM but the timing to merge this PR wold be after the v3.10 package release.

@srajiang srajiang merged commit 79e2b55 into main Feb 23, 2022
@srajiang
Copy link
Contributor

v3.10 is released! 🎉
Rebased and merging this now.

@srajiang srajiang deleted the defer-init-docs branch February 23, 2022 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wongjas wongjas wongjas left review comments

@srajiang srajiang srajiang approved these changes

@seratch seratch seratch approved these changes

Assignees

@filmaj filmaj

Labels
docs M-T: Documentation work only
Projects
None yet
Milestone
3.10.0
Development

Successfully merging this pull request may close these issues.

Add a document section about deferInitialization option when releasing v3.10
4 participants
@filmaj @seratch @wongjas @srajiang