R1C1 conversion should handle absolute A1 references

[ndench]

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

• Changes are covered by unit tests
• Code style is respected
• Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
• CHANGELOG.md contains a short summary of the change
• Documentation is updated as necessary (unecessary)

Why this change is needed?

A1 references can be made absolute in Excel by preceding the column or row with a $.

The current AddressHelper::convertToR1C1 function will ignore the $ and convert to relative R1C1 if the $currentRowNumber or $currentColumnNumber are specified, and to absolute R1C1 if not.

If you enable the R1C1 format in Excel, any reference with the $ will be converted to absolute R1C1 format.

This fix handles these references the same as Excel. Meaning that the R1C1 reference will use an absolute row or column if the $ is specified. For example $O$18 is always R18C15 regardless of whether $currentRowNumber or $currentColumnNumber are specified.

[MarkBaker]

Wouldn't it be easier to extract the $ for absolutes, the column address and row id directly through individual capture groups in the regexp rather than using substring? Then you can also eliminate the else condition in these if statements, which reduces the complexity of the code for path coverage

[ndench]

That's a good point. I originally went this way to not increase the complexity of the regex, but I think your way is cleaner. I'll push an update today.

[ndench]

I've pushed that change now. Much cleaner 👍

[MarkBaker]

Those changes to the regex don't add any significant cost to execution, as you're already executing the regex anyway; but do eliminate the substr() function calls.

The only thing I'd be tempted to change now would be to use named capture groups in the regex, so you could reference

$rowId = (int) $cellReference['row_id'];

or

if ($cellReference['absolute_column_ref'] === '$') {

as associative rather than enumerated, which adds to the readability of the code.

I've only recently started using named capture groups myself. It gives the benefit of improved readability of purpose, but It does add to the complexity of the expression. I can understand if you wouldn't want to use them though.

One last change I'd suggest would be to extract the regex to a class constant, which can then be made public, because potentially there's other places in the codebase that could use it (such as the coordinateFromString() method in Cell\Coordinate... it might be better located there as a public class constant anyway).

[ndench]

I agree that named groups are much more readable, I just stuck with non-named to match the existing code style.

I've updated it to use named groups and also moved the regex into a constant and reused it in Cell\Coordinate. Which is outside of scope for this PR, but I think the regexes should be reused constants which will minimize future bugs. Happy to undo this change if you want, but the tests still pass so it should be fine?

[MarkBaker]

Nice work, and I'm glad you were comfortable enough to do the named capture groups