check if coordinate is inside range

[AdrianBatista]

This is:

• a bugfix
• a new feature
• refactoring
• additional unit tests

Checklist:

• Changes are covered by unit tests
  • Changes are covered by existing unit tests
  • New unit tests have been added
• 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 and a link to the pull request if applicable
• Documentation is updated as necessary

Why this change is needed?

This feature can help users to check if a coordinate is inside a range. Very useful to check if a cell is inside a table for example.

[oleibman]

Thank you for the suggestion. You will need to eliminate the phpcs and php-cs-fixer errors. One way to do so would be to run the command:

composer run-script fix

at the topmost directory. You will probably have to run it twice - once for phpcs and once for php-cs-fixer.

When that is done, I would suggest some changes.

• move the throw from the bottom of your new function to the top. This will make it easier to follow the flow.
• I believe your second parameter needs to be a single cell. So, just as you test the first parameter to confirm that it is a range, you should test the second to confirm that it is not.
• You should add some test cases to demonstrate what happens when the first argument is not a range, and when the second argument is.
• I don't know what your routine will do if range or coordinate includes a worksheet name (e.g. Sheet!A1). I imagine you intend to ignore the worksheet portion, and it's okay if you do (mention this in the docBlock if so), but you'll need tests either way.
• I'm also not sure what will happen if you provide an invalid range or coordinate.

[AdrianBatista]

Thank you for the suggestion. You will need to eliminate the phpcs and php-cs-fixer errors. One way to do so would be to run the command:

composer run-script fix

at the topmost directory. You will probably have to run it twice - once for phpcs and once for php-cs-fixer.

When that is done, I would suggest some changes.

• move the throw from the bottom of your new function to the top. This will make it easier to follow the flow.
• I believe your second parameter needs to be a single cell. So, just as you test the first parameter to confirm that it is a range, you should test the second to confirm that it is not.
• You should add some test cases to demonstrate what happens when the first argument is not a range, and when the second argument is.
• I don't know what your routine will do if range or coordinate includes a worksheet name (e.g. Sheet!A1). I imagine you intend to ignore the worksheet portion, and it's okay if you do (mention this in the docBlock if so), but you'll need tests either way.
• I'm also not sure what will happen if you provide an invalid range or coordinate.

[AdrianBatista]

I run the command on my machine. But it changes almost 2k files.

Why are this happening if I just changed 2 files from master? These files should be already in correct format? Or is something missing in my environment?

[oleibman]

I don't know why that happened. Are you possibly running on Windows, and fixer changed the line endings?

[AdrianBatista]

Yes, that was it. When I added the changes git ignored those other files.

[oleibman]

What you have added is good. You are still missing what will happen when a sheet name is supplied as part of the string. Right now, you get an "Invalid cell coordinate" exception. We can, I suppose, live with that, but I'm not convinced it's the correct result. I am prepared to hear alternatives, but I think best would be:

• if neither argument supplies a sheet name, carry on as now.
• if both supply sheet names, return false if the sheet names don't match, and carry on as now (stripping out the sheet names) if they do match.
• if one supplies a sheet name and the other doesn't, return false (alternatively strip out the sheet name and carry on as now if you think that makes more sense).

Take a look at Coordinate::extractAllCellReferencesInRange to see how it extracts the sheetname and non-sheetname portion of the address. You should be able to do something similar. You also might consider (I will not require it) making your non-exception tests use the same design as the tests for extractAllCellReferencesInRange, a single test method with the test cases in an external module; this will reduce the number of test methods in CoordinateTest.

[AdrianBatista]

I've added the support to worksheet name. But I had to add another function to valid the cell reference. Please, check if it is ok now. I will add the tests after your validation.

[oleibman]

That wasn't really what I had in mind, and we already have some regexp's for coordinates. But ... I couldn't find any existing ones that were suitable, and yours is an interesting approach, so I'd like to see where it leads. I would suggest some changes:

• Make your regexp a constant (for now it can be private) rather than a literal.
• The regexp needs to be case-insensitive (i modifier is needed).
• Your function validateReferenceAndGetData need not throw an exception. It can return ['type' => 'invalid'], which will eliminate some try/catch blocks in your coordinateIsInsideRange function (and Throwable should no longer be needed).
• The worksheet name is case-insensitive, so, when you assign $data['worksheet'], you should convert to lowercase or use strcasecmp when you compare.
• The worksheet name may also be enclosed in apostrophes. You need to eliminate those (so that 'sheet'!A1 will resolve as in range Sheet!A1:A2.
• You want to account for absolute addresses (with a dollar sign before row and column). You can do this by adding [$]? in four places in your regexp, and stripping out the dollar signs when you make your assignments to $data.
• You want to use indexesFromString rather than coordinatesFromString, eliminating the columnIndexFromString call.
• There are enough existing and new cases that I think I will insist after all that you use a single test method with external test cases.

[AdrianBatista]

That wasn't what I was thinking in the begining too. But, I didn't find any other function that helps me with the worksheet name part.
About your points, I agree with all of them. Except the apostrophes one. Why the function needs to remove them? What if the worksheet name has spaces, won't it need this apostrophes?

[oleibman]

Yes, Excel needs the apostrophes sometimes. However, your routine does not. You are just comparing two sheet names for equality, and the apostrophes could interfere with that. 'Sheet'!A2 is inside sheet!A1:A3 even though the sheet name is quoted for the first and unquoted for the second. That needs to be one of your test cases. I haven't had a chance to review your latest changes yet, but should be able to get to it tomorrow.

[AdrianBatista]

Is there anything else I need to do in this pull request?

[oleibman]

I added some test cases. If I don't think of anything else that needs to be done, I will probably merge this tomorrow.

[oleibman]

Thank you for your contribution.