codeclimate / codeclimate-phpcodesniffer

Code Climate Engine for PHP Code Sniffer
MIT License
28 stars 23 forks source link

Update PHP Codesniffer #82

Closed fede-moya closed 3 years ago

fede-moya commented 3 years ago

The main purpose of the work is to allow users to run the PSR12 standard.

Sharing here some resources that have been super helpful to address this update and might be useful again in the future. Changes like renaming the old Runner.php to Executor.php and the rest of the changes can be easily understood by taking a quick look to the links shared below.

fede-moya commented 3 years ago

@filipesperandio

Seems like I don't have the enough permissions for requesting your review, that's why I ping you here.

Also, you will noticed the I have changed the base docker image alpine:edge to an specific version. While using the alpine:edge image I suffered from different dns resolutions problems which were not allowing a successful compose install, it wasn't able to fetch certain packages. After hours of checking if those packages existed and other potential explanations I ended up changing the base image and it started to work. I didn't perform any further research on that due to the fact that we are a little bit tight regarding schedule.

I addressed as CC issues as I could, there are some remaining that have been here for a long time, so I think we could temporary ignored them.

One more thing, my idea is to create a beta channel for this plugin so I think it would be nice to merge this work against a beta branch. Could you create that branch please ? 🙏🏼

One more other thing 😆 , I think we could include this work https://github.com/codeclimate/codeclimate-phpcodesniffer/pull/80/files into the release of the beta channel. WDYT ?, after merging this one I can rebase that one, test it and update that pr to point to beta. And do you have an idea of why ci/circleci is being displayed as pending 🤔 ?

filipesperandio commented 3 years ago

Seems like I don't have the enough permissions for requesting your review, that's why I ping you here.

Permissions granted, no need the fork either anymore.

Also, you will noticed the I have changed the base docker image alpine:edge to an specific version. While using the alpine:edge image I suffered from different dns resolutions problems which were not allowing a successful compose install, it wasn't able to fetch certain packages. After hours of checking if those packages existed and other potential explanations I ended up changing the base image and it started to work. I didn't perform any further research on that due to the fact that we are a little bit tight regarding schedule.

👍

will look at Circle CI now.

filipesperandio commented 3 years ago

I think the circle issue is due to the fork. we don't allow it to pass in credentials to forked PRs. Now that you have permissions for the repo, can we try pushing a non-forked branch and update the PR? (I don't recall doing it, not sure we can or a new PR is needed)

filipesperandio commented 3 years ago

Seems like we can only change base branch... so maybe a new one?

filipesperandio commented 3 years ago

beta created

filipesperandio commented 3 years ago

https://github.com/codeclimate/codeclimate-phpcodesniffer/pull/80 merged into beta

fede-moya commented 3 years ago

@filipesperandio done ! 💪🏼

fede-moya commented 3 years ago

Replaced by https://github.com/codeclimate/codeclimate-phpcodesniffer/pull/83