adael / SublimePhpCsFixer

Run php-cs-fixer code formatter to format php code from your favorite text editor
MIT License
71 stars 14 forks source link

Issue #10: add php path config support. #12

Closed gh640 closed 7 years ago

gh640 commented 7 years ago

I tried making a patch. I'd like this to be reviewed. Thanks in advance :)

adael commented 7 years ago

Thank you for your work.

I think you have made too many cs changes in the code (mostly space changes) so I would rather not merge this PR. Please conform to the current code style. And also, if it's possible, I would to change the configuration "path_php" to "php_path" which I think it's a more suitable name.

Furthermore, I would use "php_path" variable only if it's defined and respect the previous behavior so:

cmd = [path_php, path, "fix", "--using-cache=off", tmp_file]

Should become:

if php_path:
    cmd = [path_php, path, "fix", "--using-cache=off", tmp_file]
else:
    cmd = [path, "fix", "--using-cache=off", tmp_file]

Maybe this weekend, if I have time, I'll review this PR and merge after doing some changes. But feel free to make the changes if you will and then to update this PR.

Thank you again, Regards

gh640 commented 7 years ago

@adael thank you for your kind feedback.

I'm sorry, I fixed the extra line changes. Also, I changed the name path_php to php_path and changed the cmd assignment line considering the backward compatibility you told as the following.

+    cmd = [php_path] if php_path else []
+    cmd += [path, "fix", "--using-cache=off", tmp_file]

https://github.com/adael/SublimePhpCsFixer/pull/12/files

Yet, I don't stick to my PR. If it's not good to merge this PR, please don't hesitate to reject this PR and go ahead. Thank you :)

adael commented 7 years ago

Perfect! thanks you very much