Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow assoc array and generator for connection creation #689

Merged

Conversation

black-silence
Copy link
Contributor

I tried to run unittests but it's impossible on php 7.3

@ramunasd
Copy link
Member

Then why travic-ci works well? What is the error?

@lukebakken
Copy link
Collaborator

As you can see here, php 7.3.5 works just fine:

$ php --version
PHP 7.3.5 (cli) (built: Apr 30 2019 21:05:09) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.5, Copyright (c) 1998-2018 Zend Technologies
lbakken@shostakovich ~/development/php-amqplib/php-amqplib (master $=)
$ make test
./vendor/bin/phpunit
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Runtime:        PHP 7.3.5
Configuration:  /home/lbakken/development/php-amqplib/php-amqplib/phpunit.xml.dist
Warning:        The Xdebug extension is not loaded
                No code coverage will be generated.

...............................................................  63 / 766 (  8%)
............................................................... 126 / 766 ( 16%)
............................................................... 189 / 766 ( 24%)
............................................................... 252 / 766 ( 32%)
............................................................... 315 / 766 ( 41%)
............................................................... 378 / 766 ( 49%)
............................................................... 441 / 766 ( 57%)
............................................................... 504 / 766 ( 65%)
............................................................... 567 / 766 ( 74%)
............................................................... 630 / 766 ( 82%)
............................................................... 693 / 766 ( 90%)
......................................................SSSSSSSS. 756 / 766 ( 98%)
..........

Time: 6.79 seconds, Memory: 12.00MB

There were 8 skipped tests:

OK, but incomplete, skipped, or risky tests!
Tests: 766, Assertions: 947, Skipped: 8.

@lukebakken lukebakken closed this May 14, 2019
@black-silence
Copy link
Contributor Author

The error is that the contrib doc says to use $ phpunit to run tests.

What is the actual reason this is closed without comment?

@lukebakken
Copy link
Collaborator

The error is that the contrib doc says to use $ phpunit to run tests

Yes, if you have phpunit 4.8.X in your PATH, it should work. If you look at the contents of the Makefile, you can see that make test runs this command:

.PHONY: test
test:
        ./vendor/bin/phpunit

What is the actual reason this is closed without comment?

I see no reason to merge this pull request. Running tests works fine on Travis CI and on my workstation, and there is no information as to why this pull request is necessary.

@black-silence
Copy link
Contributor Author

The contrib file should be fixed to say vendor/bin/phpunit instead of just phpunit. I think it's weird to assume that someone has this stone age version of phpunit in PATH.

This PR removes a pointless limitation in create_connection. You are forcing users to give a plain list of hosts. I can't use a generator or indexed array.

I didn't think I'd have to waste time by creating an enhancement issue if I could just improve the code.

@lukebakken
Copy link
Collaborator

lukebakken commented May 14, 2019

This PR removes a pointless limitation in create_connection. You are forcing users to give a plain list of hosts. I can't use a generator or indexed array

Great. That explanation would have been very helpful in your original comment as your one-line description did not provide that information.

The contrib file should be fixed to say vendor/bin/phpunit instead of just phpunit.

Please add this change to your pull request, thanks.

I think it's weird to assume that someone has this stone age version of phpunit in PATH.

The reason for this is that we support older versions of PHP. Version 3.0.0 of this library will support only newer versions, and we can update the required phpunit version.

Finally, add tests to your pull request demonstrating that your change works as intended, and is backwards-compatible, and I will happily merge it.

@lukebakken lukebakken reopened this May 14, 2019
@lukebakken lukebakken added this to the 2.10.0 milestone May 14, 2019
@lukebakken lukebakken self-assigned this May 14, 2019
@black-silence
Copy link
Contributor Author

I'll try to remember that the commit message is not enough explanation

@lukebakken
Copy link
Collaborator

I can't merge this PR without tests demonstrating that it both is backwards compatible and supports your requirements.

@lukebakken
Copy link
Collaborator

Thank you

@lukebakken lukebakken merged commit 8d4d929 into php-amqplib:master May 22, 2019
@black-silence black-silence deleted the create_connection_foreach branch May 23, 2019 07:19
kratkyzobak pushed a commit to kratkyzobak/php-amqplib that referenced this pull request Feb 9, 2024
…on_foreach

allow assoc array and generator for connection creation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants