douggreen has suggested to merge this project fully into coder in #1774168: Coder Code Sniffer review. Let's discuss this here and collect some pros and cons.

Pros

  • Coder has a large user base and a lot more people would benefit from the sniffs we are developing here.
  • Only one issue queue to maintain.

Cons

  • Drupalcs is self-contained and has a lot of valid use cases that do not require any coder wrapping code.
  • Drupalcs follows the PHPCS coding standards to be as similar as possible to upstream, so it could be directly included with PHPCS at some point.
  • Independent development and release cycles of coder and drupalcs might be useful, we can tag slower or faster now.

Feel free to edit this, correct stuff and add further arguments!

Comments

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Title: Merging this project into coder » Merging drupalcs into coder
douggreen’s picture

douggreen commented

DCS rules can (and should) still run standalone whether it is part of coder or it's own project. If we merge, I think it's important to make sure these standalone use cases still work. I have a question. Do the standalone use cases (such as eclipse) use the druse command or phpcs? If it's the later, then I don't see any change.

Coder 7.x-2.x is already running the DCS sniffs, but it requires the user to download a second project.

I hear your concern about co-ordination of releases. It is a problem. The solution is to collaborate and communicate.

Now that coder 7.x-2.x can run DCS sniffs, a combined issue queue will probably solve some duplication, but I don't see that as a huge advantage.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Integration with other stuff like IDEs use the plain old phpcs command. Our drush integration is basically just syntactic sugar.

I will look into coder 7.x-2.x in the next days and see how it works out.

ericduran’s picture

ericduran commented

This makes sense to me.

I do have a couple of concerns it is very important to be able to run DCS outside of a Drupal environment. This is the biggest reason why this project was started.

Also I do worry that this will confuse users at 1st because a user would need to download the coder project and then extract the rules and place it somewhere which makes for a weird scenario. As long as this process stays reasonable and documented I have no issues.

It should be noted I run DCS in environments where neither drush or a full Drupal installation is available.

douggreen’s picture

douggreen commented

agreed with @ericduran. AFAIK, the current DCS requires someone to do a pear install and a symlink, I think that the process under coder would be no worse than this. But I also think that we could simplify this somewhat for users, by providing a shell script and/or a drush command to help with the setup.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

I think the symlink is not even necessary anymore, you can point to a ruleset.xml when invoking phpcs. I guess that is also possible programmatically when invoking the PHPCS classes directly.

douggreen’s picture

douggreen commented

@klausi, please take a look at my implementation ... I've bypassed using phpcs entirely and just call the base classes. I don't like spawning a new PHP process, and thus wanted to avoid phpcs. And if you look at the base UI classes, they assume argc and argv. So I had to trick it a bit with my own client class.

http://drupalcode.org/project/coder.git/blob/refs/heads/7.x-2.x:/coder_r...

It's already been suggested that I don't use the inline class here, for some coding standards. I'm not that familiar with our current standards in regards to classes. But given that inline functions, except for closures, are not part of our standards, inline classes not part of our standards makes sense too :)

das-peter’s picture

das-peter commented

I'm always for joining forces - and all possible concerns I have were mentioned already.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Ok, looks like we have an agreement here.

Now let's plan the actual inclusion into coder:
* Do we want to get added as coder maintainers, so that we can commit directly into the new coder branch? I propose that we just use the normal patch workflow in the coder issue queue for now and Doug commits stuff. If that turns out to be too slow then we can request maintainership later anyway.
* Who will work on the code transition? I have the impression that Doug is up to speed now, wants to do it, knows how PHPCS works and can do the copying. Doug, if you want us to work on that just let me know and I can make time to do it.
* We should still preserve the PHPCS coding standards that we use for our sniffs now. The reason is that we have quite some modified sniffs from upstream that are easier to update/merge if they stay more similar.
* Where do we place drupalcs inside coder? I suggest to create a "drupalcs" directory in the coder root folder and throw stuff in there.
* The drupalcs drush commands should be copied over to coder's drush integration for backwards compatibility.
* When do we deprecate drupalcs (this project)? We should wait until we have migrated all the code, the drush commands work and we have migrated the documentation. Then we need to add a message to the project page, change the maintenance category to "abandoned", move all drupalcs issues to the coder queue and close down the issue tracker here.

douggreen’s picture

douggreen commented

* coder co-maintainer would be fine with me, but I'm happy to do the heavy lifting on integrating, afterwhich time maybe you'd feel more comfortable
* I planned on doing some git magic to copy the existing tree and history into a drupalcs sub-directory, although I'm wondering if we can rename it coder_code_sniffer or some such
* I can copy the drush commands for backward comparability if you feel strongly about it, but I'm wondering if that will be helpful verses confusing. I've already integrated it into coder's drush ... the differences being that I output things integrated with all other coder results.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Thanks Doug! Preserving the git history would be awesome, but is surely no hard requirement. Renaming to coder_code_sniffer or similar is fine with me. Not sure about the drush commands and whether people are heavily using them or not - anyway not a priority right now, let's wait and see if there are complaints.

So I suggest you go right ahead with the merge - just let us know when you think it is in a usable shape. I will then follow-up with pareview.sh and switch it to coder only. I'll provide feedback and help you iron out any issues that might occur during my work on automated project application reviews.

douggreen’s picture

douggreen commented

http://drupalcode.org/project/coder.git/commit/c43676c

douggreen’s picture

douggreen commented

I tried to get the history, but it looks like I didn't. I still have work todo to make it more usable, this is just a copy of files so far.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
  • Please add a development snapshot of the 7.x-2.x branch, so that people see that there is something new and so that they can use it without git.
  • "drush coder-review --sniffer entity.install" does not work - is phpcs not implemented in coder drush yet?
  • I have removed Git write access to drupalcs so that we do not commit any more stuff to the drupalcs git repository (only ericduran hast still write access as the node author).
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
  • installation instructions have been updated to coder: http://drupal.org/node/1419988
  • added a deprecation warning to the project page and marked this project as obsolete
benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

The vimrc project configures vim for Drupal editing, and it already provides some support for both DrupalCS and Coder. I just found this issue, and I think it will be great if the two projects are coordinated: we can highlight errors with DrupalCS and then correct them automatically with Coder.

I have some catching up to do. I have not been following development of either project. With a bit of work, we can get vim, DrupalCS, and Coder all working together with drush installation scripts to make them easy to use.

Main documentation page for vimrc project: http://drupal.org/node/1389006
Screen shot of Vim + Syntastic + DrupalCS: http://drupal.org/node/1389472
Vim + Coder instructions: http://drupal.org/node/1407412#coder

patcon’s picture

patcon commented

As an avid vim user, I fully support some level of collaboration, even if just a very prominent cross-reference in the project descriptions :)

(But perhaps this should be a new thread? Also, do you have an irc handle @benjifisher? Not listed in your profile. Was hoping to speak to you about something related.)

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

@patcon:

I do not think there are cross-references on the project pages, but the main documentation pages for Vimrc, DrupalCS, and Coder are all sibling pages in the Developers' Handbook (under Contributed modules for developers), and there are cross-references in the interior documentation pages. (See the links I posted in #16 and http://drupal.org/node/1419996.)

Once I have a chance to catch up on the current state of development, I will open new issues here if I think there are changes that would make integration easier.

I rarely use IRC, but I usually reply promptly by e-mail when someone uses my d.o contact page.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Closed (won't fix)

Drupal Code Sniffer has been merged into Coder 7.x-2.x. Please move this issue to the Coder queue and reopen it if the problem still exists.