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

Add "item" interface types for "ReactionAddedEvent" #537

Merged
merged 4 commits into from
Jul 13, 2020
Merged

Add "item" interface types for "ReactionAddedEvent" #537

merged 4 commits into from
Jul 13, 2020

Conversation

dbmikus
Copy link
Contributor

@dbmikus dbmikus commented Jul 3, 2020

Summary

Add possible interface types for the items field of the ReactionAddedEvent

Requirements (place an x in each [ ])

@dbmikus
Add "item" interface types for "ReactionAddedEvent"
e0985a2 
For the "ReactionAddedEvent.item" field, add the possible interface
types that an "item" can take.
@seratch seratch added enhancement M-T: A feature request for new functionality TypeScript-specific labels Jul 3, 2020
@seratch
Copy link
Member

seratch commented Jul 3, 2020

Thanks for your contribution! Could you fix the build failure? They're just lint errors for consistency.

ERROR: /home/travis/build/slackapi/bolt-js/src/types/events/base-events.ts:615:9 - " should be '
ERROR: /home/travis/build/slackapi/bolt-js/src/types/events/base-events.ts:621:9 - " should be '
ERROR: /home/travis/build/slackapi/bolt-js/src/types/events/base-events.ts:626:9 - " should be '

https://travis-ci.org/github/slackapi/bolt-js/jobs/704572148

Except for the build issue, your changes look good to me. They're compatible with the specification.

seratch
seratch requested changes Jul 3, 2020
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.

As I mentioned in the previous command, the CI builds are failing.

seratch
seratch reviewed Jul 3, 2020
View reviewed changes
src/types/events/base-events.ts Outdated
file_comment: string;
file: string;
}

export interface ReactionAddedEvent extends StringIndexed {
type: 'reaction_added';
user: string;
reaction: string;
item_user: string;
// TODO: incomplete, should be message | file | file comment (deprecated)
Copy link
Member

Choose a reason for hiding this comment

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

Also, we can remove this comment

@dbmikus
Fix lint errors with double quotes vs single
dcfb6ce
@dbmikus
Clean up TODO comment
5124d94 
Clean up TODO comment for filling in `item` types. Mark the
`ReactionFileCommentItem` type as deprecated and link to the deprecation
notice.
@dbmikus
Copy link
Contributor Author

dbmikus commented Jul 3, 2020

Thanks for the quick review! Let me know if any other tweaks are necessary.

@codecov
Copy link

codecov bot commented Jul 3, 2020 •
edited
Loading

Codecov Report

Merging #537 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #537   +/-   ##
=======================================
  Coverage   83.13%   83.13%           
=======================================
  Files           7        7           
  Lines         593      593           
  Branches      184      184           
=======================================
  Hits          493      493           
  Misses         68       68           
  Partials       32       32

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 cf60a5b...d3ad02a. Read the comment docs.

seratch
seratch approved these changes Jul 4, 2020
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.

Thanks for the quick fix. LGTM! Other maintainers will check early next week.

@clavin clavin changed the base branch from master to main July 8, 2020 03:09
@seratch
Copy link
Member

seratch commented Jul 13, 2020

I'll merge this PR after waiting for a few more days. @aoberoi @stevengill

stevengill
stevengill approved these changes Jul 13, 2020
View reviewed changes
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.

LGTM

@stevengill stevengill merged commit 8b12e3c into slackapi:main Jul 13, 2020
@stevengill
Copy link
Member

Thanks @dbmikus! Great contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevengill stevengill stevengill approved these changes

@seratch seratch seratch approved these changes

@aoberoi aoberoi Awaiting requested review from aoberoi

Assignees
No one assigned
Labels
enhancement M-T: A feature request for new functionality semver:patch TypeScript-specific
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dbmikus @seratch @stevengill