feat(compose)!: implement compose v2 with improved typing

[alexanderankin]

relates to #306, #358

[alexanderankin]

It's taken from https://developer.hashicorp.com/terraform/language/functions/one - what about "get_first_element_or_raise"?

…

On Thu, Feb 29, 2024, 6:13 PM Jan Katins ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/testcontainers/compose/compose.py <#426 (comment)> : > +class PublishedPort: + """ + Class that represents the response we get from compose when inquiring status + via `DockerCompose.get_running_containers()`. + """ + + URL: Optional[str] = None + TargetPort: Optional[str] = None + PublishedPort: Optional[str] = None + Protocol: Optional[str] = None + + +OT = TypeVar("OT") + + +def one(array: list[OT], exception: Callable[[], Exception]) -> OT: I found this functions not so easy to understand: the naming does not imply what it does (e.g. it misses a verb), one has to read at the usage place the second argument/ the exception message to get a feeling what's happening in it. On the other hand, it doesn't really replace much duplicated code: It replaces two three-line if-clauses with two one-line one(...) calls but you need to convert the straight exception ´raiseinto alambda`. But it also has 6 lines of definition. if len(array) != 1: raise Exception(....) return array[0] vs return one(array, lambda: Exception(...)) Personally, I feel that this is less readable and the longer if version is clearer and (with two places) even overall shorter. — Reply to this email directly, view it on GitHub <#426 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACECGJFDVKAQMZ24YWDACULYV7I43AVCNFSM6AAAAABDQKELHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJQGE2DCNJQGI> . You are receiving this because you authored the thread.Message ID: ***@***.*** com>

[alexanderankin]

ok, so, oops - i can pass the tests locally on macos and ubuntu 22.04 - but not on ci. ipv4 / ipv6 advice totally welcome. maybe i take the previous approach of just returning the first possible answer, given that is passes my test of actually giving the right port for the right service or something?

[jankatins]

It's taken from https://developer.hashicorp.com/terraform/language/functions/one -
what about "get_first_element_or_raise"?

get_only_element_or_raise? But my personal style would be to inline it :-)

[carlosemart]

@alexanderankin, does it make sense to add a wait_for_healthy method that waits for the container status to be correct?

[alexanderankin]

@alexanderankin, does it make sense to add a wait_for_healthy method that waits for the container status to be correct?

im not sure, the wait method is kept from before - just the standard http wait but parametrized, from the previous api of this module

[alexanderankin]

removed fmt:off guards and it is definitely tougher on the eyes (and the with urlopen(url) as r: r.read() is no longer one concise thought expressed as one statement but rather a bloated 2 lines and an indent) - but at least it is linted the same way as the rest of the code is now.

if there are no better ideas about how to merge this, its probably better to just merge?

[totallyzen]

but rather a bloated 2 lines and an indent

I agree, but that's standard Python for you.

On the array formatting (rip pretty code) I am happy for you to try to reduce the line length to 100 and see if that helps (on another PR).

Thanks for accommodating my comments though! 🙏 💚

[totallyzen]

LGTM for a first pass. The only thing I can see is to add more tests/examples, but there is no point in delaying this PR from being merged.

You did a great job! 💚