added slack client and made the code more testable. Improved coverage #53
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… to 100% in slackResource
jepp3
requested review from
splitfeed,
flowertwig,
magnusp,
rassmate and
kindanice
Codecov Report
@@ Coverage Diff @@
## master #53 +/- ##
============================================
+ Coverage 78.75% 86.84% +8.09%
- Complexity 228 248 +20
============================================
Files 32 33 +1
Lines 640 646 +6
Branches 15 15
============================================
+ Hits 504 561 +57
+ Misses 124 73 -51
Partials 12 12
Continue to review full report at Codecov.
|
flowertwig
reviewed
Jul 8, 2019
|
|
||
| @Singleton | ||
| public class SlackResourceImpl implements SlackResource { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(SlackResourceImpl.class); | ||
| private final Slack slack; | ||
| public static final String USER_EMAIL_FROM_SLACK_IS_NULL_AND_MIGHT_BE_DUE_TO_MISSING_SCOPE_USERS_READ_EMAIL = "User email from slack is null and might be due to missing scope users:read.email"; |
There was a problem hiding this comment.
I think we should name the constants to convey the gist of the error rather than the exact words.
flowertwig
approved these changes
Jul 8, 2019
splitfeed
approved these changes
Aug 12, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Notes
Added a slack client that wraps the jslack client with obserables. This it to reduce duplication of code and make the code more readable. Note: The logic in slackResourceImpl has not been changed. Just switched to the new client.
improved the coverage to 100% in both SlackResourceImpl and SlackClientImpl. That increases the coverage to 87% for the repository.
Question:
This is one way to solve it, but there might be a smarter way?
Further work:
jSlack uses OkHttp which should have pooled connections but we still block on the rxnetty eventloop waiting for an result. Perhaps use Schedulers.io() (unbounded) or a dedicated threadpool?
Implement all methods and move out to a reusable lib, when we think its good enough.