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

Uniform the use of PHP array short syntax in all files #196

Merged
merged 5 commits into from
Feb 21, 2017

Conversation

shakaran
Copy link
Contributor

@shakaran shakaran commented Dec 22, 2016

Since there are a mix of array() and [], I think that it is a good idea uniform the use for readability issues (also test folder and examples folder are using only short syntax, only remaning the src folder).

Also now that experimental branch is consider as stable we can safely ignore old php versions (lower than 5.4) and officially deprecated the use of old PHP versions (which are EOL and unsupported too) for the sake of new php versions adoptions.

@vtsao vtsao self-assigned this Dec 23, 2016
@shakaran
Copy link
Contributor Author

shakaran commented Jan 2, 2017

@vtsao any update or comment regarding to merge this PR? I was expecting almost a quick merge, since changes are trivial

@vtsao
Copy link
Contributor

vtsao commented Jan 3, 2017

Hi @shakaran thanks for your PR. The src files marked with "This file was generated from WSDL. DO NOT EDIT." are generated with an internal tool that we need to fix on our end, so any manual changes made to those src files would be overridden the next time we generate.

All other files in your PR should be fine, so if you remove all the generated files I'll review your PR. Thanks.

@shakaran
Copy link
Contributor Author

shakaran commented Jan 3, 2017

@vtsao it seems that all the files under src and my purposed files in the PR has "This file was generated from WSDL. DO NOT EDIT.". So if you will overwrite all, I cannot make anything in my end. It should be fixed in your internal tool. Is the WDSL converter tool publicly available to use?, so I can try to modify that in other PR

@shakaran
Copy link
Contributor Author

shakaran commented Jan 3, 2017

Also FYI, I am using an automatic tool for perfom this called "php-cs-fixer" (it could be installed through composer).

I execute like:

php vendor/friendsofphp/php-cs-fixer/php-cs-fixer fix src/

And using a config file .php_cs like:

<?php

return PhpCsFixer\Config::create()
	->setRiskyAllowed(true)
	->setRules(array(
		'array_syntax' => array('syntax' => 'short'),
	))
	->setFinder(
		PhpCsFixer\Finder::create()
		->exclude('vendor')
			->exclude('tests/Fixtures')
			->in(__DIR__)
	)
;

@vtsao
Copy link
Contributor

vtsao commented Jan 3, 2017

I think you had some files like AdWordsNormalizer.php in your PR that are not generated from WSDLs.

Unfortunately our WSDL converter is not public, but I'll track it internally, although it will be a lower priority since it's the generated stub code and the issue is mainly stylistic. Agreed it should be fixed though. Thanks.

@vtsao
Copy link
Contributor

vtsao commented Jan 9, 2017

Hi @shakaran do you want to continue this pull request based on my last reply? Otherwise, I'll close it. Thanks.

@shakaran
Copy link
Contributor Author

shakaran commented Jan 9, 2017

@vtsao yes, I will update with the files that you mention. Give me 24-48h to do it

@vtsao
Copy link
Contributor

vtsao commented Jan 9, 2017

Sure no rush, just double checking. Thanks!

@shakaran
Copy link
Contributor Author

@vtsao any update on this PR?

@vtsao vtsao merged commit 7d71b95 into googleads:master Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants