-
Notifications
You must be signed in to change notification settings - Fork 11
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
Rename ruleset as "CiviCRM" to be able to have default "Drupal" standard installed as well #13
Comments
+1 on this |
OK, so this and #15 are closely inter-related. I'm not exactly sure if it's better to post my comment here or there.. but... It sounds like a question of how
Conceptually, it seems most appealing for Relatedly, I know @eileenmcnaughton sometimes expresses frustration because the Civi+Drupal rulesets drift apart, and on the surface renaming would sorta harden that split. But I think maybe the real problem is the confusion that arises from mixing them (eg choosing the "Drupal" style in PHPStorm will give you warnings on a "CiviCRM" style codebase). So if we bite the bullet and make the split cleaner, perhaps that would address the same concern. |
Hmm - I think my concern is that I want us to be periodically increasing our compliance - ie every now & then getting us in compliance with a new rule & locking it in. There are some places (class naming) where we are legitimately different but in general the drupal rules are ones we should adhere to & it's only because we haven't made existing code compliant that we don't |
I think the suggestion 3 is the right way to approach this - build on top of Hopefully it would make maintenance easier in the long run and also gives us the ability to easily and cleanly do things like lint extension code automatically as part of extension review processes (speeding this up is a win as we can get more extensions approved for in-app!) it lets developers pull the standards into pretty much every editor/IDE these days (the amount of faff to get these standards in VSCode is a nightmare and knocks out the Drupal standards!) but more importantly hopefully it actually makes it easier to maintain/update as we're just worried about maintaining our code on top of the Drupal coder package as opposed to maintaining a fork. I've started working locally on detangling the Drupal specific bits and instead referencing them from Should I be rebasing this off of the 3.x.x version? |
From the change log, it sounds like the fundamental difference in 3.x.x is the target version of phpcs: https://www.drupal.org/project/coder/releases/8.x-3.0 and of course a large number of subsequent patches to the sniffs/rules. The main thing to look out for is the time required for balancing out the new sniffs and the code in core repos. Hopefully it can be trimmed without too much difficulty. This does seem like a good opportunity to modernize the phpcs version. |
Then that's the direction I'll go. Replicate where we are currently on 3.x.x in a new branch so we can test it against core code repos and see where we're at! |
hey @eileenmcnaughton @totten @homotechsual @sluc23 - interested in your thoughts on this: https://lab.civicrm.org/michaelmcandrew/civicrm-coding-standards/-/issues/1 |
This ruleset is a variation of the Drupal default ruleset, but installed in phpcs under Drupal name.
This blocks the possibility of installing default Drupal ruleset with the same name, to validate Drupal modules in the same machine.
Is there a way to have both ruleset installed in the same box where Drupal modules and CiviCRM core/extensions are developed?
If not, wouldn't make sense the rename this ruleset as CiviCRM ?
The text was updated successfully, but these errors were encountered: