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

Improve App initialization error logs + docs #1250

Merged
merged 3 commits into from
Jan 20, 2022
Merged

Improve App initialization error logs + docs #1250

merged 3 commits into from
Jan 20, 2022

Conversation

srajiang
Copy link
Member

@srajiang srajiang commented Dec 18, 2021 •
edited
Loading

Summary

  • Improves App initialization error case logging
  • Adds links to relevant documentation for better troubleshooting
  • Tweaks OAuth documentation to specify required oauth installer options.
  • Additions to help improve code readability make maintaining easier

Test

  • Easiest way would be to use npm link these changes to one of your sample apps and try out different combos of App options.

Requirements (place an x in each [ ])

@srajiang srajiang self-assigned this Dec 18, 2021
@srajiang srajiang added the enhancement M-T: A feature request for new functionality label Dec 18, 2021
@srajiang srajiang added this to the 3.9.0 milestone Dec 18, 2021
@codecov
Copy link

codecov bot commented Dec 18, 2021 •
edited
Loading

Codecov Report

Merging #1250 (990cd48) into main (2a97007) will not change coverage.
The diff coverage is 77.77%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1250   +/-   ##
=======================================
  Coverage   73.22%   73.22%           
=======================================
  Files          17       17           
  Lines        1438     1438           
  Branches      431      431           
=======================================
  Hits         1053     1053           
  Misses        300      300           
  Partials       85       85
Impacted Files Coverage Δ
src/App.ts 85.05% <77.77%> (ø)

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 2a97007...990cd48. Read the comment docs.

seratch
seratch approved these changes Dec 18, 2021
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.

Left one comment about the change on the boolean test part. All other changes sound great 👍

src/App.ts Show resolved Hide resolved
srajiang
srajiang commented Dec 20, 2021
View reviewed changes
src/App.ts Outdated Show resolved Hide resolved
mwbrooks
mwbrooks approved these changes Jan 4, 2022
View reviewed changes
docs/_basic/authenticating_oauth.md
Comment on lines +12 to +13
* `clientId`, `clientSecret`, `stateSecret` and `scopes` (required)
* An `installationStore` option with `storeInstallation` and `fetchInstallation` handlers defined for storing installation data to a database *(optional, but highly recommended for production)*
Copy link
Member

Choose a reason for hiding this comment

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

Nice improvement on clarity. I always appreciate when arguments are explicitly "required" or "optional"

src/App.ts Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
misscoded
misscoded approved these changes Jan 6, 2022
View reviewed changes
src/App.ts Outdated Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
src/App.ts Outdated Show resolved Hide resolved
filmaj
filmaj reviewed Jan 12, 2022
View reviewed changes
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Sorry I'm late to the party here! Agree with the comments here, there are a few small tweaks left to some of the wording and error messaging that would improve this PR. That being said, thanks for putting the time into making these changes - already a huge improvement over what was there before!

src/App.ts Outdated Show resolved Hide resolved
@srajiang srajiang merged commit a3a612c into slackapi:main Jan 20, 2022
@srajiang srajiang deleted the app-init-logging branch January 20, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@filmaj filmaj filmaj left review comments

@seratch seratch seratch approved these changes

@misscoded misscoded misscoded approved these changes

@mwbrooks mwbrooks mwbrooks approved these changes

@stevengill stevengill Awaiting requested review from stevengill

Assignees

@srajiang srajiang

Labels
enhancement M-T: A feature request for new functionality
Projects
None yet
Milestone
3.9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@srajiang @seratch @mwbrooks @filmaj @misscoded