feat: implement conversations.inviteShared

Aenimus commented 1 year ago

Pull Request Guidelines

These are recommendations for pull requests. They are strictly guidelines to help manage expectations.

PR preparation

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

Should this be an issue instead

[ ] is it a convenience method? (no new functionality, streamlines some use case)
[ ] exposes a previously private type, const, method, etc.
[ ] is it application specific (caching, retry logic, rate limiting, etc)
[ ] is it performance related.

API changes

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

no tests, if you're adding to the API include at least a single test of the happy case.
If you can accomplish your goal without changing the API, then do so.
dependency changes. updates are okay. adding/removing need justification.

Examples of API changes that do not meet guidelines:

in library cache for users. caches are use case specific.
Convenience methods for Sending Messages, update, post, ephemeral, etc. consider opening an issue instead.

lorenzoaiello commented 2 months ago

@Aenimus , it looks like I may not have permission to push to your forked branch to re-queue the GitHub Action CI checks.

Would you mind committing a no-op in the forked-branch so the checks will run?

$ git commit --allow-empty -m "Trigger GitHub Actions"
$ git push

Thanks!

Aenimus commented 2 months ago

Hi @lorenzoaiello,

I did as you have asked, but nothing seems to have changed (I'm assuming the message does not have to match identically).

lorenzoaiello commented 2 months ago

Hi @lorenzoaiello,

I did as you have asked, but nothing seems to have changed (I'm assuming the message does not have to match identically).

Thanks! Since its a fork actions have to be approved by a maintainer but they did queue up correctly and I just approved the request 🚀

Can you take a look at the failing tests please? It looks like there may be a conflict when running the whole test suite.

Aenimus commented 2 months ago

Hi @lorenzoaiello,

I'd be happy to address the tests if I understood the error. I can run things locally just fine. I also don't see any other references to /conversations.inviteShared.

Aenimus commented 2 months ago

I just ran again from root: go test -v -race ./...

No failures or errors. Please advise.

lorenzoaiello commented 2 months ago

@Aenimus , I'll try to do some digging to see if I can figure out what might be going on.

Aenimus commented 2 months ago

Hi @lorenzoaiello,

I still cannot replicate the CI errors, and I don't understand what could cause them.

Do you have any further insights?

lorenzoaiello commented 1 month ago

@Aenimus - can you please try to rebase your branch and see if that makes it reproducible?

It seems like there was a commit after your original fork that added a conflicting test/endpoint registration. (I'm not sure if the CI is doing something non-obvious during checkout that is merging code)

Aenimus commented 1 month ago

@lorenzoaiello So the problem is that someone implemented this 3 months after I did, and that version was merged.

I suppose you can close this.

lorenzoaiello commented 1 month ago

@lorenzoaiello So the problem is that someone implemented this 3 months after I did, and that version was merged.

I suppose you can close this.

It would appear so - apologies I missed that as well until now :( - Thank you for your effort though, its much appreciated!

slack-go / slack