codeclimate / codeclimate-phpcodesniffer

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

Updating of standards? #60

Closed xendk closed 7 years ago

xendk commented 7 years ago

Currently it looks like the images is only rebuilt when committing new standards/fixes.

At least the Drupal standard gets running updates, which means that it'll quickly get outdated.

How to handle this?

maxjacobson commented 7 years ago

Hi @xendk. We're happy to accept PRs that bump the versions of dependencies. In codeclimate-eslint, we have a greenkeeper bot that automatically opens PRs when dependencies have received updates. Is there something similar for PHP?

xendk commented 7 years ago

Tried looking but I couldn't find a PHP service like greenkeeper. https://www.versioneye.com/ was closest, but no automatic PRs as far as I can tell.

One could create a cron job that does a composer update and uses the hub command to create a PR if composer.lock changed, but it would need somewhere to run.

maxjacobson commented 7 years ago

Thanks for looking into that. I wonder if https://docs.travis-ci.com/user/cron-jobs/ could be wrangled into filling that role... 🤔

xendk commented 7 years ago

Don't see why not? All it needs is to run composer update and git diff --quiet composer.lock and this: https://gist.github.com/willprice/e07efd73fb7f13f917ea

But before we get ahead of ourselves here, how about pinning checkers? I haven't been able to find a way to do this, but I know people will want to do this. Nobody wants their rating to go through the floor because the standard used had a major update and it was automatically pulled in.

maxjacobson commented 7 years ago

We generally take the approach that rolling updates are good, because then you don't need to think about keeping your linters up-to-date, but I can see how that doesn't work for everyone and every engine. In some other engines, we've pinned by using "channels". For example: https://docs.codeclimate.com/docs/rubocop#section-using-rubocop-s-newer-versions . We could see if that makes sense for phpcodesniffer.

In the meantime, we'd be happy to accept a PR if you want to update any standards for the default channel.

xendk commented 7 years ago

Well, I'm leaning towards rolling updates too, but my CTO doesn't like the idea that a developer adding small feature to an older project suddenly have to deal with new issues in old code. Does CC take what code was actually changed into account? I believe Scrutinizer tries to, with mixed success. You wouldn't have to have some links to posts that argues the point, would you?

I'm not sure that channels would work well. They seems more like branches than tags to me? And considering how many standards the container has, it could get quite messy.

I was somewhat surprised, considering how much the home page talks about containers and how to build your own, that using your own custom image isn't an option? If it was an option to just specify an image and tag, we could just fork this repo and pin down to our hearts content. Companies using their own custom standards/tools might also like that idea.

maxjacobson commented 7 years ago

Ah, I see.

Does CC take what code was actually changed into account? I

Yes! For pull request and branch comparisons, we show you new and fixed issues. When an engine changes, we make sure to re-analyze the merge base so that the comparison is between two "snapshots" that were analyzed using the same engines. I'm not sure where to point you for further information on that, but happy to answer any further questions about it.

I was somewhat surprised, considering how much the home page talks about containers and how to build your own, that using your own custom image isn't an option?

That's true that we don't currently allow that. It is possible to build your own engine and use it via the CLI, but using it in the cloud currently requires making it available to everyone, and some collaboration with our engineering team.

We're working on some engines, like our recently-release grep engine and others to come soon, which are super customizable, and may also help your team check for exactly what you're interested in checking.

xendk commented 7 years ago

Oh. So what exactly happens? Lets say I create a new PR and the engine was updated since last the master was analyzed, and now it flags a new thing as an error, but in code I haven't touched in the PR?

I would suggest you consider the possibility of using custom images. Considering that you already run the analyzers without network access, I think the challenge is limited to resource usage on your cloud. Besides being able to solve odd problems like my own, I would think that it would be good for take-up on shared engines too. Well, my 0.2 , anyway.

But there could be another workaround: If the prepare step was able to fetch a zip file and unpack it, we could use that to install, say, https://github.com/klausi/coder/archive/8.2.12.zip and point to the standard within. Not as sexy, but workable.

On scrutinizer we just have the coder module as a dev dependency in composer.json, and it does a composer install. I find that a bit iffy, but my boss likes it.

While I like the concept of evergreen analyzers, I need to dig up some good arguments for the boss.

maxjacobson commented 7 years ago

Oh. So what exactly happens? Lets say I create a new PR and the engine was updated since last the master was analyzed, and now it flags a new thing as an error, but in code I haven't touched in the PR?

Sort of. It will notice that new issue, but it will notice it in both snapshots: the one for your feature branch and the one for your default branch. Because it's present on both branches, we won't consider it a "new" issue in the context of your pull request, and we won't require you to fix it to have a green pull request.

Regarding your other comments, I really appreciate you articulating that idea and I'm going to capture it as feedback for our product team to consider.

xendk commented 7 years ago

That sounds excellent. I'll try and work on the boss.

maxjacobson commented 7 years ago

😄. Going to close this for now, but happy to answer any other questions you or the boss may have.