Minor changes

[sagikazarmark]

Repository:

• [x] Please use this .gitattributes file
• [x] Please use this .gitignore file
• [x] Please use this .travis.yml file (Don't forget to add the before_script which starts the PHP server, scripts should be on new line as yaml struct)
• [x] Please add Change Log info
• [x] Please use this CONTRIBUTING guide (note the extra email address)
• [x] Please update the security section based on the php-http/httplug repo (note the extra email address in the security section)
• [x] Please remove the VERSION file. Are there any reasons it is there?
• [x] Documentation should be added instead of the Usage section

CS:

• [x] Please remove the file header right after the php opening tag, I see no value in it
• [x] SPL Exceptions should not be imported, but used from the root namespace to avoid too much importing
• [x] Not sure if @api makes sense here
• [x] Please add more newlines in the code. Some parts of it is hard to read
• [x] Please use line comments instead of block comments

@joelwurtz SocketClient might also be affected by the repository issues, haven't checked though.

[mekras]

this .gitignore ignores composer.lock. But how about this?

[mekras]

Also I changed travis.yml according #2.

[sagikazarmark]

this .gitignore ignores composer.lock. But how about this?

From the site you linked:

Commit your application's composer.lock (along with composer.json) into version control.

Note: For libraries it is not necessary to commit the lock file, see also: Libraries - Lock file.

It is a common belief that composer.lock should be version controlled. It is only the case for applications which is the "end-user" of composer. In libraries, composer.lock rather a pain as it can cause frequent conflicts.

[mekras]

Please remove the VERSION file. Are there any reasons it is there?

The reason is machine readable version number in a fixed location for different automation needs. OK, I'll remove it.

[sagikazarmark]

Just saw another thing in #5: we are using semantic versioning which means versions should follow x.z.y pattern, like 1.0.0

[mekras]

Documentation should be added instead of the Usage section

Create separate issue #5.

[mekras]

Please remove the file header right after the php opening tag, I see no value in it

At least phpDocumentor will treat this as an error. Also this header contains brief info about package. Is it really so important to removed it?

[mekras]

Please add more newlines in the code. Some parts of it is hard to read

What parts?

[mekras]

Not sure if @api makes sense here

Any harm from it? Yes, currently there is only one class here. But this can be changed in the future. Also this class is really a part of library API.

[sagikazarmark]

At least phpDocumentor will treat this as an error. Also this header contains brief info about package. Is it really so important to removed it?

You sure? The class can still have it's own header.

[sagikazarmark]

For example this: https://github.com/php-http/curl-client/blob/master/src/CurlHttpClient.php#L125

[mekras]

You sure? The class can still have it's own header.

I'm sure. Checked with the latest version.

[sagikazarmark]

Any harm from it?

No, but I can't see the added value (yet).

[sagikazarmark]

Well, we don't have a plan to use phpdocumentor (for now) but this might change in the future.

[mekras]

while (count($allHeaders) > 0 && '' === $lastHeaders)

Is quite a simple condition. Splitting it into two lines make no sense for me.

[sagikazarmark]

Not into two lines, but adding new lines before and after (the loop block).

[mekras]

Oh, understand.

[mekras]

@sagikazarmark, so what about .travis.yml, headers, @api and new lines? Should I change something? Or can leave it as is?

[sagikazarmark]

Travis: yes please, as we are trying to use the same travis config everywhere so it is easier to update. The important points: it should be testing framework independent: use composer test-ci command instead of phpunit. Also please not that the env vars are in one line in the build matrix: it is not by accident, multiline is considered to be invalid by the travis linter.

Headers: since apigen doesn't require it, I think we should drop them. Since there is only one class in the file, description should be in front of the class. As I can see, apigen uses it anyway.

FYI: I am working on a solution which will autogenerate apidoc and store it in a central git repo, served by Github Pages.

Api: I don't mind, although I can't see the point.

New lines: would be nice, but you can leave it as is. Scrutinizer will panic and suggest changes.

[mekras]

OK.

Since there is only one class in the file, description should be in front of the class.

I'll add more files for #3.

[mekras]

All done.