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

Revert behavior change to ApplicationContext.respond, make send_response and send_followup explicit in their usage #656

Merged
merged 4 commits into from
Dec 30, 2021

Conversation

krittick
Copy link
Contributor

Summary

This reverts the change made to ApplicationContext.respond() in #645, but moves its behavior from that PR to ApplicationContext.send_response() instead. This creates a more consistent experience in that both send_response and send_followup now explicitly do what their method names suggest.

While ApplicationContext.respond() may still be worth renaming, its current naming does make sense from a language point of view in that "responding" to something can consist of a "response" or a "follow-up" - the original context remains the same in both cases.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, typehinting, examples, ...)

@krittick krittick changed the title Revert behavior change to ApplicationContext.respond, make send_response and send_follow explicit in their usage Revert behavior change to ApplicationContext.respond, make send_response and send_followup explicit in their usage Dec 28, 2021
Dorukyum
Dorukyum previously approved these changes Dec 28, 2021
@hypergonial
Copy link
Contributor

I honestly do not think that this sufficiently adresses the naming conflict between send_response and respond, I don't think there is much point in switching them around, people will find it confusing regardless.

@krittick
Copy link
Contributor Author

I honestly do not think that this sufficiently adresses the naming conflict between send_response and respond, I don't think there is much point in switching them around, people will find it confusing regardless.

To clarify, this PR is not intended to address the naming of the function. It simply reverts the massive breaking change to .respond (which IMO was merged much too quickly with barely any discussion), and keeps the new behavior from #645, but on send_response instead.

We can discuss naming in #649 but for now the best user experience would be to not break every bot's usage of slash commands in a single commit without first having a clear path forward on where to take things. This PR accomplishes that while still including the other benefits of #645.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants