-
Notifications
You must be signed in to change notification settings - Fork 22
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
Async es client factory function #302
base: main
Are you sure you want to change the base?
Conversation
Hi there,
Did you manage to check out the updates/ comments back and would you be willing to merge into main?
Thanks,
Sam
From: Grzegorz Śliwiński ***@***.***>
Reply to: ClearcodeHQ/pytest-elasticsearch ***@***.***>
Date: Wednesday, 21 April 2021 at 10:54
To: ClearcodeHQ/pytest-elasticsearch ***@***.***>
Cc: Samuel Wilson ***@***.***>, Author ***@***.***>
Subject: Re: [ClearcodeHQ/pytest-elasticsearch] Async es client factory function (#302)
@fizyk commented on this pull request.
________________________________
In src/pytest_elasticsearch/factories.py<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL0NsZWFyY29kZUhRL3B5dGVzdC1lbGFzdGljc2VhcmNoL3B1bGwvMzAyI2Rpc2N1c3Npb25fcjYxNzM3MjI4Mw==&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=OHJBdzB6NXV3NkMrcXoxZHZUREhhZ25MU0xlQnFVZ2JxaXlWamY5dWRXWT0=&h=9012922b44a54b1c8b3a72feffefef40>:
@@ -42,7 +42,7 @@ def return_config(request):
for option in options:
option_name = 'elasticsearch_' + option
conf = request.config.getoption(option_name) or \
- request.config.getini(option_name)
+ request.config.getini(option_name)
Do not change unrelated lines. Especially if something is just a formatting difference.
I plan to employ black to actually keep an eye on the format, but until then, if it passes pylint and pycodestyle, do not reformat it if you're not changing it.
________________________________
In src/pytest_elasticsearch/factories.py<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL0NsZWFyY29kZUhRL3B5dGVzdC1lbGFzdGljc2VhcmNoL3B1bGwvMzAyI2Rpc2N1c3Npb25fcjYxNzM3NjU2Mw==&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=L2w4TklFV0luTzJXU3lNcmhDRlF6UWxxZWZaM0lZWnlPQnNrcm5meUNJZz0=&h=9012922b44a54b1c8b3a72feffefef40>:
@@ -190,3 +190,32 @@ def drop_indexes():
return client
return elasticsearch_fixture
+
+
+def async_elasticsearch(process_fixture_name):
I'm wondering what would be the use case for the async fixture? To use the async connection for the code that expects it?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL0NsZWFyY29kZUhRL3B5dGVzdC1lbGFzdGljc2VhcmNoL3B1bGwvMzAyI3B1bGxyZXF1ZXN0cmV2aWV3LTY0MDg3MTgyNw==&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=T3pHa1l4VjdUR2E4cmlRTzA0UkRBN2ROWmVjNEtxQW9QSG92Z21oWFpQdz0=&h=9012922b44a54b1c8b3a72feffefef40>, or unsubscribe<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL25vdGlmaWNhdGlvbnMvdW5zdWJzY3JpYmUtYXV0aC9BU0xCSVFLUjY3M1RPNkIzSUpFVURWTFRKMk9EVEFOQ05GU000M0dEU05RQQ==&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=NnpsMlhGblJmSUc3QUFsVWx0TUYzVjd1eFlyQVhIbXQrYU15VlRydVpwOD0=&h=9012922b44a54b1c8b3a72feffefef40>.
|
@samwnapier yes, it looks good. However I'm in the process of migrating all the pytest plugins I manage to github actions and it'll take me some time to focus on this. Worst case scenario, it'll be moved to examples in readme, as I'm not sure about requiring additional library, or it'll be a fixture that'll get activate whether the asyncio is present or not (so totally depending on the developer's requirements) |
Okay sure. Below is an example of the tests that should probably also go in readme. The first is unchanged but the second requires the pytest-asyncio import so that the function can be marked as async in the decorator.
Do you have a rough time expectation of when you may merge or integrate this into the package? Just so I can get a rough idea on my timelines too.
***@***.***D74259.C9997940]
From: Grzegorz Śliwiński ***@***.***>
Reply to: ClearcodeHQ/pytest-elasticsearch ***@***.***>
Date: Thursday, 6 May 2021 at 09:10
To: ClearcodeHQ/pytest-elasticsearch ***@***.***>
Cc: Samuel Wilson ***@***.***>, Mention ***@***.***>
Subject: Re: [ClearcodeHQ/pytest-elasticsearch] Async es client factory function (#302)
@samwnapier<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL3NhbXduYXBpZXI=&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=bkRzZ05uRG1uZlg1dmJxako0Zk8xMG5yNjcybmFiWnpjWkpnS0dzQ0UwQT0=&h=cdcb0020df4b4129b7940aa27229bfff> yes, it looks good. However I'm in the process of migrating all the pytest plugins I manage to github actions and it'll take me some time to focus on this. Worst case scenario, it'll be moved to examples in readme, as I'm not sure about requiring additional library, or it'll be a fixture that'll get activate whether the asyncio is present or not (so totally depending on the developer's requirements)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL0NsZWFyY29kZUhRL3B5dGVzdC1lbGFzdGljc2VhcmNoL3B1bGwvMzAyI2lzc3VlY29tbWVudC04MzMzMjQyOTQ=&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=L21yUjNGUkFCU1hJTzMrWitSeExxVFJtZis2cXNrV3dFWjdDTkZxTjZIaz0=&h=cdcb0020df4b4129b7940aa27229bfff>, or unsubscribe<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL25vdGlmaWNhdGlvbnMvdW5zdWJzY3JpYmUtYXV0aC9BU0xCSVFQVVNSTEEzSU1ONFhUU0VaRFRNSkZITkFOQ05GU000M0dEU05RQQ==&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=OWU0ZzJFd2tCR0o5a3Npb01rNmxQZVdEcGt5Wm9ibWFHbmtpS3htRkFVOD0=&h=cdcb0020df4b4129b7940aa27229bfff>.
|
@samwnapier okay, let's do it this way so that it's activated if the asyncio library is present. |
Hey Grzegorz,
Sorry for the delay, had a few other big issues I needed to resolve that took priority. There would be some complications with using same function name and then having an activation or such due to the nature of asyncio and the decorators that are needed to declare it as async. I think having two separate functions is the way to go with this. Okay so just need to add a test and then should be okay to merge in?
Thanks,
Sam
From: Grzegorz Śliwiński ***@***.***>
Reply to: ClearcodeHQ/pytest-elasticsearch ***@***.***>
Date: Friday, 7 May 2021 at 16:59
To: ClearcodeHQ/pytest-elasticsearch ***@***.***>
Cc: Samuel Wilson ***@***.***>, Mention ***@***.***>
Subject: Re: [ClearcodeHQ/pytest-elasticsearch] Async es client factory function (#302)
@samwnapier<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL3NhbXduYXBpZXI=&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=bkRzZ05uRG1uZlg1dmJxako0Zk8xMG5yNjcybmFiWnpjWkpnS0dzQ0UwQT0=&h=2776975c038649be933f4513ef544a80> okay, let's do it this way so that it's activated if the asyncio library is present.
I'd also require a test utilising that fixture, otherwise there's a risk we'd break it with some code update.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL0NsZWFyY29kZUhRL3B5dGVzdC1lbGFzdGljc2VhcmNoL3B1bGwvMzAyI2lzc3VlY29tbWVudC04MzQ1NjgxNjQ=&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=MEdVd2Rjb1pnTDl3Tjg1L1lHd3BxRjJLeDdQY1c1UjRVSDI5Uk5pV3YzVT0=&h=2776975c038649be933f4513ef544a80>, or unsubscribe<https://eu-west-1.protection.sophos.com?d=github.com&u=aHR0cHM6Ly9naXRodWIuY29tL25vdGlmaWNhdGlvbnMvdW5zdWJzY3JpYmUtYXV0aC9BU0xCSVFPV05BSUU0T0pUMjRTVVNHTFRNUUU1M0FOQ05GU000M0dEU05RQQ==&i=NWZkY2FlNGVmYzJlOTgwZWQ5YzNkMzkw&t=em44SDFWWElXVWF6bm5aOEduaktXRDc0THd0WEtVbGtzNmVqZmIzKzU2VT0=&h=2776975c038649be933f4513ef544a80>.
|
@samwnapier yes, plus the conditional activation of the fixture :) (Don't want to have the asyncio as a hard dependency) |
Adding async ES client function.
Changes proposed.