Implement missing INFO function

[RobinvanderVliet]

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?

Provide an explanation of why this change is needed, with links to any Issues (if appropriate).
If this is a bugfix or a new feature, and there are no existing Issues, then please also create an issue that will make it easier to track progress with this PR.

[oleibman]

I'm going to have to think about this for a bit. Your work on INFO is fine. However, the tests which you are removing from TypeAttributePreservationTest are problematic. The tests, of course, no longer succeed because INFO would no longer be unimplemented. But they are designed to test important functionality in PhpSpreadsheet. I do not know if there is an existing unimplemented function which we might be able to substitute. Or perhaps we need to add our own unimplemented function. Or perhaps removing the tests is not as important as I think. Regardless, I will need time.

[oleibman]

I think a kludgey solution will work here. Add a static boolean property $infoSupported, defaulting to true, to Information/Info. If false, return the result of Functions::DUMMY; else return the result from your new code.

Next, restore the deleted tests from TypeAttributePreservationTest. Add a setUp to set infoSupported to false, and a tearDown to set it back to true.

With this approach, we can implement and test your code. We don't have to search for a substitute unimplemented function for our existing tests, an exercise that we might have to repeat if we at some point implement the substitute. We preserve our existing tests. This seems like a good overall result despite the kludginess.

[RobinvanderVliet]

Thank you for your feedback, I've implemented your suggestion.

[oleibman]

Please add a cosmetic change - use the following docBlock for $infoSupported:

    /**
     * @internal
     */

This will exclude the property from being listed in the official API documentation.

There are 3 INFO function arguments which you don't support - directory, origin, and release. You have good reasons for not supporting them, but you are returning a VALUE error if they are used. That is what you would return for, say, xxxxxxx, and it's somewhat misleading to return the same result for values that Excel itself doesn't support vs. values that Excel supports but PhpSpreadsheet can't support. Can you instead return something like "unavailable" for the 3 (or maybe something like "$A:$A$1" for origin, the format that Excel uses and which I readily admit I do not understand)?

[oleibman]

According to https://support.microsoft.com/en-us/office/info-function-725f259a-0e4b-49b3-8b52-58815c69acae, there are 3 additional arguments - memavail, memused, and totmem, all of which return #N/A. You may as well add these too.

[RobinvanderVliet]

Thank you for your feedback. Can you take another look?

[oleibman]

I apologize for not noticing this earlier. It is, I hope, the last change I'll ask you to make. INFO takes only 1 argument, but, in Information/Info, you have defined a second argument $cell for it. Please remove that argument from the function definition and the doc-block and the use statement for Cell.

[RobinvanderVliet]

I added 'passCellReference' => true to FunctionArray.php because I need the cell for the implementation of INFO("numfile").

[oleibman]

Ah, clever, and the test you just added demonstrates that use. Thanks.