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

PHPCodeBeautifierBear: Add PHPCodeBeautifierBear #2156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bhavishyagopesh
Copy link

@bhavishyagopesh bhavishyagopesh commented Dec 8, 2017

Adds PHPCodeBeautifierBear that supports detection and
fixing of php code.

Closes #1771

For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!

Checklist

  • I read the commit guidelines and I've followed
    them.
  • I ran coala over my code locally. (All commits have to pass
    individually.
    It is not sufficient to have "fixup commits" on your PR,
    our bot will still report the issues for the previous commit.) You will
    likely receive a lot of bot comments and build failures if coala does not
    pass on every single commit!

After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.

Please consider helping us by reviewing other peoples pull requests as well:

The more you review, the more your score will grow at coala.io and we will
review your PRs faster!

@bhavishyagopesh bhavishyagopesh force-pushed the feat/phpcbf branch 2 times, most recently from 3b118e3 to ca19565 Compare December 20, 2017 07:08
@gitmate-bot gitmate-bot added size/M and removed size/S labels Dec 20, 2017
@bhavishyagopesh
Copy link
Author

@SanketDG I made the required changes

.travis.yml Outdated
@@ -103,6 +103,9 @@ before_install:
sed -i.bak '/^coala/d' requirements.txt
sed -i.bak '/^mypy-lang/d' requirements.txt bear-requirements.txt
fi
# On ubuntu:14.04 this seems the best way to install `phpcbf`
# With ubuntu:16.04 `php-codesniffer` contains `phpcbf`
- sudo pear install PHP_CodeSniffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use the newer composer here?

also, you have to put this in a new shell script, .ci/deps.composer.sh, look at other package manager shell scripts to see how it is done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but still this needs to be moved preferably to .ci/deps.sh

@SanketDG
Copy link
Member

SanketDG commented Dec 20, 2017

Just realised its already there https://github.com/coala/coala-bears/blob/master/.ci/deps.apt.sh#L28 since we have the codesniffer bear, so you dont need anything in .travis.yml ideally. there are a bunch of other php linting tools that should be served by composer, I have opened #2187 for that, but thats a separate issue

@bhavishyagopesh
Copy link
Author

bhavishyagopesh commented Dec 20, 2017

@SanketDG No that was the problem actually with ubuntu:14.04(which the ci uses if it were 16.04 it would work), php-codesniffer(for ubuntu:14.04) package does not come with phpcbf binary(or atleast does not puts it into $PATH), that's why I had to pear-install it separately

@SanketDG
Copy link
Member

@bhavishyagopesh that makes sense, it probably carries an old version, I am up for using pear right now and then move to composer when #2187 is implemented.

@bhavishyagopesh
Copy link
Author

@SanketDG If you say, I tested the composer setup locally just now(inside ubuntu docker), I can add the deps.composer.sh file...than we'll probably not need php-codesniffer in apt-packages

@bhavishyagopesh bhavishyagopesh force-pushed the feat/phpcbf branch 2 times, most recently from 2c1f830 to 2e9e2bd Compare December 20, 2017 12:49
@SanketDG
Copy link
Member

we'll probably not need php-codesniffer in apt-packages

yup that would be a separate PR

@bhavishyagopesh bhavishyagopesh force-pushed the feat/phpcbf branch 5 times, most recently from c09d423 to 8ca602c Compare December 26, 2017 17:50
@bhavishyagopesh bhavishyagopesh force-pushed the feat/phpcbf branch 2 times, most recently from 79c0ce1 to e223ae1 Compare December 29, 2017 08:14
@bhavishyagopesh
Copy link
Author

@SanketDG the appveyor ci fails because of missing phpcbf executable, (and also it's not having pear).So how should I add that dependency?

return os.path.join(os.path.dirname(__file__), name)


def load_testfile(name):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a look at other bears these 2 methods (get_testfile_path and load_testfile) are used in the test files of bear and not the file that wraps the linter. So these 2 function names didn't make sense to me when I first read your code and then I realized that you use them to load the sample config file (if I'm not wrong). So I think it would be better to rename these methods as get_configfile_path and load_configfile respectively. It would make much more sense to someone who reads your code in the future. 😄

Adds `PHPCodeBeautifierBear` that supports detection and
fixing of php code.

Closes coala#1771
Copy link
Contributor

@newbazz newbazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it takes the pear to be pre installed which is not the case, so u need to install the pear.

@SanketDG
Copy link
Member

SanketDG commented Jan 4, 2018

I have no idea how to add pear to windows, if you cant figure it out from googling, its probably not supported well enough, so just feel free to skip the tests on windows.

@@ -39,6 +39,7 @@ install:
# Check that we have the expected version and architecture for Python
- "python --version"
- "python -c \"import struct; print(struct.calcsize('P') * 8)\""
- "pear install PHP_CodeSniffer"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add the skip decorator to your test class, so you dont need to install pear onto windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

PHP Code Beautifier and Fixer
7 participants