Skip to content

Conversation

b4sus
Copy link
Contributor

@b4sus b4sus commented Dec 18, 2023

Follow up of the issue #95.

Please, pay attention to the verify/cert configuration, this is something I am not very familiar with nor could I test it.

Things to check:

  • verify/cert configuration
  • headers

Copy link

cla-checker-service bot commented Dec 18, 2023

💚 CLA has been signed

@pquentin
Copy link
Member

@b4sus Wow, this is exciting! Thanks for working on this.

I'd like to set the expectations here by noting that this is a large change that will require multiple rounds of review, and that I will be slow because of the holidays season coming up. My goal is to have this merged by the end of January.

I haven't looked at the code in any meaningful way but I noticed it uses asyncio. However, I'd like to use anyio instead because it will get us Trio support for free, which is the main reason to support HTTPX for me.

Also, can you please sign the CLA?

Thanks again!

@pquentin
Copy link
Member

The DefaultType | (float | None) syntax is only valid in Python 3.10 and above, unless you add from __future__ import annotations. But if we do that, we should do it in all files, by adding add_imports = "from __future__ import annotations" to our isort configuration here: https://github.com/elastic/elastic-transport-python/blob/main/setup.cfg and running pyupgrade. As an added bonus, this sounds like a nice way to get your first contribution in, so that I don't have to click "Approve and run" every time you make a change. (Maybe I can tinker with the GitHub settings to avoid that though.)

The other option is to stick to typing.Union.

@pquentin
Copy link
Member

Are you OK if I submit some small changes to this branch myself?

@b4sus
Copy link
Contributor Author

b4sus commented Dec 19, 2023

Of course, go for it.

b4sus and others added 7 commits December 19, 2023 12:03
All nodes are named the same way.
They are arguably worse, but make comparisons easier. Switching to
better names should be done in a separate change for all nodes.
A previous comment used `exc` by mistake.
@pquentin
Copy link
Member

Could not help myself, this is turning into bigger changes. Hope that's still OK 🙈

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me now. I'm happy to merge if you are. 🎉

@pquentin pquentin merged commit b96d158 into elastic:main Feb 8, 2024
@b4sus b4sus deleted the 95-HTTPX-support branch February 28, 2024 07:58
@mmahacek mmahacek mentioned this pull request Apr 1, 2024
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.

2 participants