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

feat: Update request.py in order to allow Streaming responses #737

Merged
merged 12 commits into from
May 15, 2024

Conversation

TheoBessel
Copy link
Contributor

(Example : needed to build correct ChatGPT XBlocks)

@openedx-webhooks
Copy link

openedx-webhooks commented Apr 11, 2024

Thanks for the pull request, @TheoBessel! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 11, 2024
@mphilbrick211
Copy link

Hi @TheoBessel! Thanks for this contribution! Please let me know if you have any questions regarding submitting a CLA form.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Apr 12, 2024
@mphilbrick211
Copy link

Hi @TheoBessel! Thanks for this contribution! Please let me know if you have any questions regarding submitting a CLA form.

Hi @TheoBessel! Just following up on this. Please let us know when you submit your CLA form. Thanks!

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Apr 17, 2024
@TheoBessel
Copy link
Contributor Author

TheoBessel commented Apr 17, 2024

Hi @TheoBessel! Thanks for this contribution! Please let me know if you have any questions regarding submitting a CLA form.

Hi @TheoBessel! Just following up on this. Please let us know when you submit your CLA form. Thanks!

Hi, @mphilbrick211 thanks for your reply ! I have just submitted my CLA form !

@TheoBessel TheoBessel changed the title Update request.py in order to allow Streaming responses feat!: Update request.py in order to allow Streaming responses Apr 17, 2024
@mphilbrick211
Copy link

Hi @TheoBessel! Thanks for this contribution! Please let me know if you have any questions regarding submitting a CLA form.

Hi @TheoBessel! Just following up on this. Please let us know when you submit your CLA form. Thanks!

Hi, @mphilbrick211 thanks for your reply ! I have just submitted my CLA form !

Great! The change should be reflected within 24 hours. When you re-run your checks after that, the check next to the CLA requirement should turn green.

@TheoBessel
Copy link
Contributor Author

@mphilbrick211 Do I have to do anything else now ?

@mphilbrick211
Copy link

Hi @TheoBessel - I will look into getting the checks re-enabled for you!

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Apr 22, 2024
@TheoBessel
Copy link
Contributor Author

Hi @TheoBessel - I will look into getting the checks re-enabled for you!

@mphilbrick211 Thanks a lot for your help !

@TheoBessel
Copy link
Contributor Author

@mphilbrick211 Hello ! Any news ?

@mphilbrick211
Copy link

@mphilbrick211 Hello ! Any news ?

Hi @TheoBessel - thanks for checking in, and for your patience. The checks should be enabled today. Apologies for the delay.

@TheoBessel
Copy link
Contributor Author

@mphilbrick211 @nedbat Hello ! I've edited my code because it didn't pass the tests. Should I also to edit my commit messages or they are okay ? Let me know. I'll also make a PR on edx-platform and xblock-sdk in order to make the change I've proposed usable on these sides.
Have a nice day !

@TheoBessel
Copy link
Contributor Author

@mphilbrick211 Hello ! Have you seen my last comment ?

@e0d
Copy link

e0d commented May 1, 2024

@TheoBessel I notice there are some commit-lint failures. Please note that we use conventional commits across Open edX projects. You can read about the details here. Can you please amend your commit messages to follow our standard?

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label May 1, 2024
@TheoBessel TheoBessel force-pushed the patch-1 branch 2 times, most recently from 1306c49 to bafbb16 Compare May 6, 2024 12:57
@TheoBessel
Copy link
Contributor Author

@e0d I guess it is good now ?

TheoBessel and others added 2 commits May 6, 2024 16:31
* chore: Updating Python Requirements

* build: Update codecov to use the repository upload token.

---------

Co-authored-by: Irtaza Akram <[email protected]>

Update request.py
@TheoBessel
Copy link
Contributor Author

@e0d Updated the too long commit message and added some unit tests, it should pass all workflows now (I hope)

@TheoBessel
Copy link
Contributor Author

@e0d I hope this time it will be good (I removed a test I wrote and that made no sense for Streaming Responses and that was failing because of that)

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label May 7, 2024
@TheoBessel
Copy link
Contributor Author

@e0d ?

@TheoBessel
Copy link
Contributor Author

Can somebody here merge my pull request ? It passes all the checks now.

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label May 14, 2024
@mphilbrick211
Copy link

@TheoBessel - thanks for flagging! I have requested review.

@ormsbee
Copy link
Contributor

ormsbee commented May 14, 2024

@TheoBessel: Please resolve conflicts and squash your commits and I'll merge.

@TheoBessel
Copy link
Contributor Author

@TheoBessel: Please resolve conflicts and squash your commits and I'll merge.

I'm sorry, but I don't really know what you're talking about. I don't see any conflicts ans my commits were already amended in order to have correct messages. 🤔

@ormsbee
Copy link
Contributor

ormsbee commented May 15, 2024

Ah, sorry, I saw this:

Screenshot 2024-05-15 at 12 24 53 PM

And it didn't click to me that you did a merge instead of a rebase off of master (it's fine, it's just not what I'm used to). I'll squash and merge now.

@ormsbee ormsbee merged commit 7d72ad0 into openedx:master May 15, 2024
12 checks passed
@openedx-webhooks
Copy link

@TheoBessel 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@ormsbee ormsbee changed the title feat!: Update request.py in order to allow Streaming responses feat: Update request.py in order to allow Streaming responses May 15, 2024
@ormsbee
Copy link
Contributor

ormsbee commented May 15, 2024

Note for anyone looking at this later: I wasn't paying attention to the top line of the squashed commit message, and merged it in with the feat!: it got from the original title of this PR, rather than feat:, which is what it should be. This PR did not take away anything or break backwards compatibility.

I'll make a new PR to bump the version of this package and tag it for PyPI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants