chore: refactor unimplemented method test macro

[ohmayr]

This PR refactors the macro for testing not implemented methods of rest transport.

For now, the macro is guarded and only generates tests for sync REST not implemented methods. Once we implement the relevant logic in async rest, the guard can be removed. (This will be addressed in a follow up PR).

[parthea]

Added non-blocking comments. Feel free to follow up in a later PR

[vchudnov-g]

I support having this rest-checking failsafe here, it but it does seem somewhat redundant/uninformative. At the very least least you should error if transport is wrong:

{% if 'rest' not in transport %}
{{ raise GenerationError }}
{% else %}
...
{% endif %}

More generally, though, I would support changing the name of this macro so it can be used for both REST and gRPC. Obviously we would do the gRPC refactoring later, not in this PR. WDYT? If this sounds feasible, consider editing as below, and let's file an issue. This would help us ensure we're thorough in our tests and consistent for both transports.

{% macro run_tests_for_config(service, transport, is_async) %}
{% if 'grpc' in transport %}
{{ raise GenerationError("gRPC tests will be moved here") }}
{% else %} {# rest #}
...
{% endif %}

[ohmayr]

1. I've filed an issue and linked it to follow up with adding gRPC tests here as suggested.
2. I've updated the macro name to be more generic.

I don't think we need to raise a generation error for gRPC tests since it would add unnecessary toil in the test suite (i.e. this suite shouldn't be run against gRPC transport which is why we're skipping for it).

I think It's fine to skip generation for it as long as we have a tracking bug to include gRPC tests.

[vchudnov-g]

Thanks!

I don't think we need to raise a generation error for gRPC tests since it would add unnecessary toil in the test suite (i.e. this suite shouldn't be run against gRPC transport which is why we're skipping for it).

The idea is to just have a fail-safe to ensure we're indeed skipping it. It protects us from our own future programming errors. I've found this defensive practice valuable.

Not a blocker for this PR, but I still say it's a nice to have.

[ohmayr]

I agree that it's nice to have a fail-safe. Let's sync on this offline and we can address this as a follow up if needed.

[vchudnov-g]

Can you have the macro return True and False as booleans rather than strings?

[ohmayr]

No. Macros can only return strings and this is the best thing I could come up with.

[vchudnov-g]

Right, right.

I guess what I was thinking would be more clearly described in terms of defining the functionality of this macro in Python proper (eg def is_supported(method, is_async):...), so the invocations below would call something like {% if is_supported(method, is_async) %} ... {% endif %} without the string comparison, which grates a little. That would be more elegant. But if you expect this macro to be temporary anyhow, we can skip it. What you have is fine.

[vchudnov-g]

Right, right.

I guess what I was thinking would be more clearly described in terms of defining the functionality of this macro in Python proper (eg def is_supported(method, is_async):...), so the invocations below would call something like {% if is_supported(method, is_async) %} ... {% endif %} without the string comparison, which grates a little. That would be more elegant. But if you expect this macro to be temporary anyhow, we can skip it. What you have is fine.

[vchudnov-g]

Thanks!

I don't think we need to raise a generation error for gRPC tests since it would add unnecessary toil in the test suite (i.e. this suite shouldn't be run against gRPC transport which is why we're skipping for it).

The idea is to just have a fail-safe to ensure we're indeed skipping it. It protects us from our own future programming errors. I've found this defensive practice valuable.

Not a blocker for this PR, but I still say it's a nice to have.

[vchudnov-g]

OK, thanks for having a policy!

I was also referring to consistency with pre-existing macros, even before this PR. Making macro naming consistent overall would be a nice cosmetic clean-up, but obviously not critical. We can follow up at some point (though honestly, it seems like this will be near the bottom of our list for a good time, since it's not that important).