search

jakubkulhan / bunny

Performant pure-PHP AMQP (RabbitMQ) sync/async (ReactPHP) library
MIT License
706 stars 102 forks source link

Typehint to DateTimeInterface to allow DateTimeImmutable values #96

Closed fritz-gerneth closed 4 years ago

fritz-gerneth commented 4 years ago

This is for the PHP->AMQP type conversion only. Most of our code uses DateTimeImmutable types ( as our dates are not mutable :)). This safes us from having to cast them. From what I understood this part is not auto-generated. This should also be no BC break.

WyriHaximus commented 4 years ago

From what I understood this part is not auto-generated. This should also be no BC break.

Had to double check this, but indeed it doesn't look auto-generated :+1: . Do you think adding a unit test to enforcing this would make sense? Just in the case when we do generate code again we don't accidentally drop it

fritz-gerneth commented 4 years ago

I see some tests expect a specific RabbitMQ configuration - is there some Docker image / definitions file for the expected setup?

WyriHaximus commented 4 years ago

I see some tests expect a specific RabbitMQ configuration - is there some Docker image / definitions file for the expected setup?

I'll get onto that, there isn't one but there are some things we could improve and having a single command to run all tests locally should be one of them.

WyriHaximus commented 4 years ago

@fritz-gerneth Someone beat me to it: https://github.com/jakubkulhan/bunny/pull/97

fritz-gerneth commented 4 years ago

@fritz-gerneth Someone beat me to it: #97

Sorry to hear that ;) Will add the test once #97 is merged then.

WyriHaximus commented 4 years ago

@fritz-gerneth Someone beat me to it: #97

Sorry to hear that ;) Will add the test once #97 is merged then.

Hey I'll take it once all is green :D

mermshaus commented 4 years ago

Sorry, I didn’t notice this until now. I had most parts of #97 lying around for quite a while before I finally managed to split things into commits and to prepare the PR. :slightly_smiling_face: My main intention is to fix issues with disconnection of the synchronous client (see eg. #93). (There are some more places in the tutorial where it is done wrong (e: or maybe incomplete).) This gave me a lot of gray hair in a larger test suite that ran into random heartbeat timeouts from earlier unrelated tests after 10 minutes or so.

WyriHaximus commented 4 years ago

@fritz-gerneth #97 is in, great thanks to @mermshaus for that 👍

WyriHaximus commented 4 years ago

@mermshaus Yeah I understand that, it's a great package, but it can use some fixes and improvements for certain cases. ANd I appreciate your work on improving it 👍 !

fritz-gerneth commented 4 years ago

Tests are added now. Test setup works pretty flawless 👍