Improve test coverage and function docs: math_expression

Lets get #2827100: Improve test coverage and function docs fixed and applied first, then work on other improvements.

Lets reroll this, now that #2827100: Improve test coverage and function docs has been committed.

All looks good, but.

+++ b/includes/math-expr.inc
@@ -1,388 +1,955 @@
+  private function nfx($expr) {

This is a very very long method? Perhaps break it out into smaller sub methods? (Perhaps it can be addressed at later date, including the pfx method)

Some more things

1. +++ b/tests/math_expression.test
   @@ -20,142 +20,302 @@ class CtoolsMathExpressionTestCase extends DrupalWebTestCase {
   +      // Equal if both an infinity .
   

   There is a space before the period

2. +++ b/tests/math_expression.test
   @@ -20,142 +20,302 @@ class CtoolsMathExpressionTestCase extends DrupalWebTestCase {
   -    $math_expression = new ctools_math_expr();
   ...
   +    $this->assertFloat(3.1415926535898, $math_expr->evaluate('pi'));
   

   Looking at the ctools_math_expr class pi and e are declared using the php function pi() and exp(1) why are they not tested against these values? Is there something I'm missing here?

This didn't get into 1.13, maybe it will get into 1.14.

We need to try getting this to RTBC.

I tried re-testing the patch from #15 to see what phpcs would report (I noticed there are a lot of coding standards issues in the patch), but I see the patch no longer applies.

Here's a re-roll of the patch ctools-n2830393-incl1958538-15.patch from #15 - let's see what the testbot reports then I will re-roll it again with coding standards fixes. It's not clear to me how useful this is, because it mixes up changes from this issue and from #1958538: Improve math expression engine, but it's a start.

One question though:
I see this patch adds the constant 'c' (speed of light), although that is not discussed above.
All the other defined constants are dimensionless - pure numbers like pi and e. 'c' would be the only constant with dimensions, which naturally brings with it the question of what units are used for this constant and how are the units stored/communicated to users and how conversion of units is handled in math expressions. Because of this, I feel adding 'c' should be removed from this patch, as it is a feature request which requires some decision on how to carry around and use dimensioned constants.

And here's the same patch as #19, with most of the coding standards fixed. See the interdiff for changes. (Entirely doc comments, comments, and type hints). I did NOT change class names or class properties to upperCamel, as I feel that would be out of scope for this issue, so there are going to be some remaining coding standards warnings.

1. +++ b/ctools.api.php
   @@ -152,30 +154,55 @@ function hook_ctools_api_hook_alter(&$list) {
   + * One usecase would be to create your own function in your module and
   

   should say use case

2. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   + * @file
   

   Classes don't need @file notation

3. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +      $fnn = $matches[1];
   

   Could this function be named more clearly than $fnn?

4. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +    foreach ($this->userfuncs as $fnn => $dat) {
   

   Variable name could do with being more descriptive.

5. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +          $fnn = $matches[1];
   

   Variable naming again

6. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +            $fdef = $this->funcs[$fnn];
   

   Variable naming.

7. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +          // yay... recursion!!!!
   

   Needs a capital letter at start.

I believe a coding standards cleanup has been done on all the source so this should cover some of the simple things like capital letters, but there are many abbreviated variables that would be better of being more descriptive as not all developers will recognise them particularly non native English speakers.

@darrenwh:

1) Fixed.

2) From the Drupal coding standards:

The @file doc block MUST be present for all PHP files, with one exception: files that contain a namespaced class/interface/trait, whose file name is the class name with a .php extension, and whose file path is closely related to the namespace (under PSR-4 or a similar standard), SHOULD NOT have a @file documentation block.

In this case, class ctools_math_expr is in a file called math-expr.inc. There is no .php extension, the class name is not the same as the file name, and the class is not namespaced, so it absolutely SHOULD have a @file comment according to the coding standards. That's why the PHPCS coding standards check does not complain about @file here.

3)

Could this function be named more clearly than $fnn?

Yes, but it's currently used 20 times in math-expr.inc. This issue didn't name that function, and it didn't change how that function is used. I think the name should be changed, but I think that's outside the scope of this issue.

4), 5), 6) Same as 3).

7) Fixed.

I would appreciate a comment about the 'c' variable usage that I asked about above, and a review of the substance of the issue. This patch already fixes more than 700 coding standards issues, and it only introduced ONE new coding standards issues (usecase -> use case, fixed in this new patch), so I suggest that further coding standards problems be a part of a separate issue so that we can move this along.

CTools PHP 5.3 tests are still failing because of #2783113: Using trait without PHP 5.4 requirement breaks Drupal CI of all modules that require UUID, but PHP 5.4+ tests work. Test failure was not because of this patch.

1. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +  protected $userfuncs;
   

   This should be in lower camel case

2. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +  protected $constvars;
   

   This should be in lower camel case

3. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +  protected $simplefuncs;
   

   This should be in lower camel case

4. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +  protected $binaryops;
   

   This should be in lower camel case

5. +++ b/includes/math-expr.inc
   @@ -1,391 +1,960 @@
   +  private function nfx($expr) {
   

   Wonder if this would be better off in its own class?

As I said in #24,

I would appreciate a comment about the 'c' variable usage that I asked about above, and a review of the substance of the issue. This patch already fixes more than 700 coding standards issues, and it only introduced ONE new coding standards issues (usecase -> use case, fixed in this new patch), so I suggest that further coding standards problems be a part of a separate issue so that we can move this along.

So, back to "Needs review"

If you want to "fix" CTools by making all the class variables camel case like phpcs wants, then please open a new issue where you can fix all 148 of those at once. That's way off topic for this issue.

OK have a RTBC then have created https://www.drupal.org/project/ctools/issues/2988049

I'm putting up a tests only patch to smoke test this.

@TR I don't have enough domain knowledge to answer your question about "dimensioned constants" but if it doesn't make sense I'd agree to your assessment to remove the example.

Test failure in #31 was because yesterday there was a Drupal core commit that introduce short array syntax into core - this makes ALL PHP 5.3 tests fails. See #2482549: Ignore node_module folder in core to use Drupal with npm/grunt/nodejs

Thanks @TR for the heads up. I’ll wait till that commit is fixed.

Maintainers don't appear to want to commit updated tests with broken code, or updated code without tests, and as there is a lot of overlap/cross-blocking I've taken the latest and greatest from here and merged it with #1958538: Improve math expression engine in an attempt to pull them both together and prevent future conflicts, so closing this as a duplicate.