Skip to content
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

Adding tests for AddressHelper::convertFormulaToA1 #2076

Closed
ndench opened this issue May 8, 2021 · 4 comments
Closed

Adding tests for AddressHelper::convertFormulaToA1 #2076

ndench opened this issue May 8, 2021 · 4 comments

Comments

@ndench
Copy link
Contributor

ndench commented May 8, 2021

This is:

- [ ] a bug report
- [ ] a feature request
- [x] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

While working on #2060 I noticed that the AddressHelper::convertFormulatToA1 doesn't have any tests.

I'd like to write these tests because I think this is a very valuable function that contains a lot of logic. However there are some parts of the function that I don't understand.

The main things I'm not sure of are:

  1. What sort of functions start with of:
  2. What sort of functions contain " and why do we only convert the non-quoted parts?

Here's the function, with my questions in comments blocks.

/**
 * Converts a formula that uses R1C1 format cell address to an A1 format cell address.
 */
public static function convertFormulaToA1(
    string $formula,
    int $currentRowNumber = 1,
    int $currentColumnNumber = 1
): string {
    if (substr($formula, 0, 3) == 'of:') {
        /*
         * I don't understand this entire if block.
         * What sort of formula starts with 'of:' and contains double quotes?
         * Why are we simply removing square brackets and full stops?
         * If this formula is in R1C1 format, shouldn't we be converting something to A1 somwhere?
         */

        $formula = substr($formula, 3);

        $temp = explode('"', $formula);
        $key = false;
        foreach ($temp as &$value) {
            //    Only replace in alternate array entries (i.e. non-quoted blocks)
            if ($key = !$key) {
                $value = str_replace(['[.', '.', ']'], '', $value);
            }
        }
    } else {
        /*
         * Again, why does the formula contain quotes?
         */

        //    Convert R1C1 style references to A1 style references (but only when not quoted)
        $temp = explode('"', $formula);
        $key = false;
        foreach ($temp as &$value) {
            //    Only replace in alternate array entries (i.e. non-quoted blocks)
            if ($key = !$key) {
                /*
                 * Inside this if block, I understand. It simply locates all R1C1 references and converts them A1.
                 */

                preg_match_all('/(R(\[?-?\d*\]?))(C(\[?-?\d*\]?))/', $value, $cellReferences, PREG_SET_ORDER + PREG_OFFSET_CAPTURE);
                //    Reverse the matches array, otherwise all our offsets will become incorrect if we modify our way
                //        through the formula from left to right. Reversing means that we work right to left.through
                //        the formula
                $cellReferences = array_reverse($cellReferences);
                //    Loop through each R1C1 style reference in turn, converting it to its A1 style equivalent,
                //        then modify the formula to use that new reference
                foreach ($cellReferences as $cellReference) {
                    $A1CellReference = self::convertToA1($cellReference[0][0], $currentRowNumber, $currentColumnNumber);
                    $value = substr_replace($value, $A1CellReference, $cellReference[0][1], strlen($cellReference[0][0]));
                }
            }
        }
    }
    unset($value);
    //    Then rebuild the formula string
    $formula = implode('"', $temp);

    return $formula;
}

If someone would be able to explain these parts to me and potentially provide some example functions, that would allow me to write much more effective tests.

@MarkBaker
Copy link
Member

Hopefully simple answers to our questions:

What sort of functions start with of:
Both OpenOffice OASIS, and SpreadsheetML use an of: prefix for formulae, although the actual formatting of the formula is different between the two file formats... in this case, the convertor is being used for SpreadsheetML formulae. SpreadsheetML is the short-lived and largely unused format that MS tried to adopt between BIFF-8 and OfficeOpenXML. There are some examples in the sample Xml files used for Reader tests.
of:=[.B4]+[.C4]
of:=SUM([.B1:.B4])

If you save a file open in Excel as XML Spreadsheet 2003 (*.xml) that's what you'll get.

Why are we simply removing square brackets and full stops?
If this formula is in R1C1 format, shouldn't we be converting something to A1 somwhere?
It isn't actually R1C1 format, the method heading is a bit of a misnomer, because it's actually handling both the SpreadsheetML format formulae, and formulae using R1C1 references.
Breach of SRP!

What sort of functions contain " and why do we only convert the non-quoted parts?
This is by far the quickest way of handling formulae that contain string literals, where we want to modify the formula in some way, but avoid changing the content of those string literals.
Example:

=CONCAT("Result of formula expression =R1C1+R1C2 is: ", R1C1+R1C2)

@ndench
Copy link
Contributor Author

ndench commented May 10, 2021

Ah right, that makes complete sense now, thanks!

Do you think this function should be split into two separate functions (one for SpreadsheetXML and one for A1)? That would require the callers to know which format the formulas are in. Is that a problem?

@MarkBaker
Copy link
Member

It should be, even just separate methods inside the same class, because they're both reformatting addresses, but from different formats

@ndench
Copy link
Contributor Author

ndench commented Jun 3, 2022

Closed by #2086

@ndench ndench closed this as completed Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants