feat: add support for explain prepared query API endpoint #368
Conversation
|
Hi @larrytamnjong, |
|
Sure👍, thanks for the feedback.
…On Wed, Oct 2, 2024, 4:19 PM Mehdi Nassim KHODJA ***@***.***> wrote:
Hi @larrytamnjong <https://github.com/larrytamnjong>,
As a best practice, I suggest keeping this PR as a draft while it's still
in progress (for example, when tests are missing)
Thanks
—
Reply to this email directly, view it on GitHub
<#368 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A5DVXR46PSK53VJQ6CO7M7DZZQFILAVCNFSM6AAAAABPHW4IKCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBYHE2TGNJSGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@naskio I've added the tests. Please take a look when you have time. Thanks! |
There was a problem hiding this comment.
Hi @larrytamnjong, thanks for the PR.
I've left just a single comment to remove unnecessary lines, it looks good otherwise.
Consul.Test/PreparedQueryTest.cs
Outdated
| Assert.True(query.Name == "my-query"); | ||
| Assert.True(query.Service.OnlyPassing == true); | ||
|
|
||
| query = null; |
There was a problem hiding this comment.
I know that is the way it has been done in this test, but nulling the response before assigning new values serves no purpose. Can I ask you to remove all the null assignments from the tests?
There was a problem hiding this comment.
@marcin-krystianc thanks for the feedback let me do that..
|
@marcin-krystianc all the null assignments have been removed. |
Add support for Explain Prepared Query API endpoint in Consul.Net (implementation only, tests pending)
#367
@naskio