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

Add bookmarks.<add|edit|remove|list> support #1044

Merged
merged 7 commits into from Apr 26, 2022

Conversation

winston-stripe
Copy link
Contributor

Summary

Add support for the bookmarks.<add|edit|remove|list> endpoints https://api.slack.com/apis/bookmarks-api

Motivation

Implements bookmarks per request in #1036

PR preparation

Run make pr-prep from the root of the repository to run formatting, linting and tests.

  • Done!
API changes

Since API changes have to be maintained they undergo a more detailed review and are more likely to require changes.

This PR does include API changes since it exposes new endpoints for accessing

All implemented methods include tests

@winston-stripe winston-stripe changed the title Add bookmarks.* support Add bookmarks.<add|edit|remove|list> support Mar 16, 2022
@zchee zchee self-requested a review March 18, 2022 16:18
@zchee
Copy link
Contributor

zchee commented Mar 18, 2022

@winston-stripe will review soon

@zchee
Copy link
Contributor

zchee commented Mar 18, 2022

@winston-stripe url.alues.Has added from go1.17, could you another approach?
https://pkg.go.dev/net/url#Values.Has

@winston-stripe
Copy link
Contributor Author

@zchee sg, and i've removed the dependency on url.Values.Has. Thanks!

@zchee
Copy link
Contributor

zchee commented Mar 18, 2022

@winston-stripe Passes test. Thanks!
Will review to core logic.

@zchee
Copy link
Contributor

zchee commented Mar 22, 2022

Hmm, BTW go1.14 or older is too old. I want to protect the master branch with passes go1.15~go1.17(go1.18 in the future) CIs but haven't permission.

@kanata2 Could you add me to the owner role?

@winston-stripe
Copy link
Contributor Author

lmk if there's anything else you need from me here :)

@kanata2
Copy link
Member

kanata2 commented Mar 23, 2022

@zchee granted you a repo's admin role. 😄

@zchee
Copy link
Contributor

zchee commented Mar 27, 2022

@winston-stripe will review again. sorry for the late

@R167
Copy link
Contributor

R167 commented Apr 2, 2022

hi @zchee, have you had a chance to review this again yet?

@zchee
Copy link
Contributor

zchee commented Apr 2, 2022

@R167 sorry for the late.
will review next week (after tomorrow).

@winston-stripe
Copy link
Contributor Author

hi, just bumping this PR again 😃 (appreciate your time!)

bookmarks.go Show resolved Hide resolved
bookmarks.go Outdated Show resolved Hide resolved
bookmarks.go Outdated Show resolved Hide resolved
bookmarks.go Outdated Show resolved Hide resolved
@kanata2 kanata2 added the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Apr 16, 2022
@winston-stripe
Copy link
Contributor Author

@kanata2 thanks for the checks! I fixed those spelling mistakes for when you have a chance to look again.

Copy link
Member

@kanata2 kanata2 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@kanata2 kanata2 removed the feedback given When a review has been conducted and awaiting the response from the comitter(s) label Apr 21, 2022
@kanata2 kanata2 added this to the v0.11.0 milestone Apr 21, 2022
@winston-stripe
Copy link
Contributor Author

one more followup: anything more to do here, or is it just "wait for the v0.11.0 milestone thing" and PRs will be merged then?

@kanata2 kanata2 merged commit d28b536 into slack-go:master Apr 26, 2022
@kanata2
Copy link
Member

kanata2 commented Apr 26, 2022

merged

@winston-stripe winston-stripe deleted the add-bookmarks branch April 6, 2023 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants