Open
Conversation
fizyk
reviewed
Apr 21, 2021
Author
|
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>.
|
Member
|
@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) |
Author
|
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>.
|
Member
|
@samwnapier okay, let's do it this way so that it's activated if the asyncio library is present. |
Author
|
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>.
|
Member
|
@samwnapier yes, plus the conditional activation of the fixture :) (Don't want to have the asyncio as a hard dependency) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding async ES client function.
Changes proposed.