Project Idea - Refactoring Codebase and Adding Some Coding standard

yashrajbothra · March 11, 2020, 8:40pm

Honorable Mentors,

As i was exploring the codebase from a few days now i was not sattisfied with the current coding standards. Many files are not well linted and some files have unnecessary comments and some having deprecated code.Till now mostly i was into interface part and that could be refactored very much there might be more part of codebase which could have potintial to refactor we can discuss that below.But for Coding Standard as we are mostly using PHP in the codebase ( ) we can introduce PEAR Coding style with the help of phpcs.
The Goal here is to have clean and readable code for contributors so that there would be a great Future for OpenEMR

Thanks you
Yash Bothra

stephenwaite · March 11, 2020, 9:09pm

hi @yashrajbothra, could see this as part of Project - Automated Testing, check out how in the codebase travis.yml and the ci directory is handling the phpcs if you haven’t already

robert.down · March 13, 2020, 12:39pm

We already have a coding standard in place, a big issue is making legacy code comply. But if I remember we did some automated linting and should have a pretty decent coverage rate. See https://www.open-emr.org/wiki/index.php/Development_Policies#PHP

yashrajbothra · March 13, 2020, 1:06pm

Hii @robert.down @stephenwaite I totally agree on the point that we have coding standard but it seems the whole codebase is not following it

stephenwaite · March 13, 2020, 2:05pm

hi @yashrajbothra, correct, there’s only certain files that are being scanned rn, like can be see in travis.yml

brady.miller · March 14, 2020, 1:21am

hi,

Regarding php_codesniffer run by travis.

The only places that are skipped by the php_codesniffer are the gacl directory and a couple classes:

github.com

openemr/openemr/blob/master/ci/phpcs.xml#L33-L36


<exclude-pattern>*/gacl/*</exclude-pattern>
<exclude-pattern>*/library/classes/fpdf/*</exclude-pattern>
<exclude-pattern>*/library/classes/smtp/*</exclude-pattern>
<exclude-pattern>*/library/classes/PDF_Label.php</exclude-pattern>

Note that we are NOT fully PSR2 compliant though and are skipping below rules:

github.com

openemr/openemr/blob/master/ci/phpcs.xml#L5-L14


<rule ref="PSR2">
  <exclude name="PSR1.Classes.ClassDeclaration"/>
  <exclude name="Squiz.Classes.ValidClassName"/>
  <exclude name="PSR1.Methods.CamelCapsMethodName"/>
  <exclude name="Squiz.Scope.MethodScope"/>
  <exclude name="PEAR.Functions.ValidDefaultValue"/>
  <exclude name="PSR2.Classes.PropertyDeclaration"/>
  <exclude name="Generic.NamingConventions.UpperCaseConstantName"/>
  <exclude name="Squiz.WhiteSpace.ScopeClosingBrace"/> <!-- ~100 files need fixing from this -->
</rule>

brady.miller · March 14, 2020, 1:22am

Quick aside, this line can be removed if anybody wants a quick PR:

github.com

openemr/openemr/blob/master/ci/phpcs.xml#L30


  <rule ref="Generic.WhiteSpace.ScopeIndent">
    <properties>
      <property name="ignoreIndentationTokens" type="array">
        <element value="T_COMMENT"/>
        <element value="T_DOC_COMMENT_OPEN_TAG" />
      </property>
    </properties>
  </rule>


  <exclude-pattern>*/vendor/*</exclude-pattern>
  <exclude-pattern>*/bower_components/*</exclude-pattern>
  <exclude-pattern>*/node_modules/*</exclude-pattern>
  <exclude-pattern>*/public/assets/*</exclude-pattern>
  <exclude-pattern>*/gacl/*</exclude-pattern>
  <exclude-pattern>*/library/classes/fpdf/*</exclude-pattern>
  <exclude-pattern>*/library/classes/smtp/*</exclude-pattern>
  <exclude-pattern>*/library/classes/PDF_Label.php</exclude-pattern>
</ruleset>

stephenwaite · March 14, 2020, 7:44pm

robert.down · March 14, 2020, 8:48pm

I would recommend moving the non-compliant code towards the standard we already have in place versus attempting to begin following a new standard