Refactor Xls Reader #4118
Merged
Refactor Xls Reader #4118
+5,643
−6,156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I have been having some time-out problems with php-cs-fixer in my environment (no problems *yet* in Github). Bringing that up with them, the first thing they suggested was that it might be due to very large modules. The largest module they use in testing is a bit over 1,000 lines, and we have about 16 that exceed that. The biggest of these is Xls Reader; at 7,647 lines, it is more than 2,000 lines longer than its nearest competitor (Calculation), and at least 5 times larger than fixer's max. It's not clear to me that breaking it up will actually solve my problem. On the other hand, perhaps it is time to do some re-factoring anyhow; as an example, changing the parsing of tfunc/tfuncv values from enormous select statements to indexing constant arrays, possibly external, is something that ought to have happened long ago. This turned out to be easier than I had thought. Breaking it into sub-modules each of which can access each other's protected properties and methods was fairly straightforward. There's a bit of overhead in having to allocate new classes, but Xlsx Reader has been doing that all along (without the protected access part). I've managed to remove about 2,900 lines from Reader/Xls, scattering those among 2 existing and 6 new source modules. I'm not sure I've chosen the best possible, or most maintainable, approach. I'm not sure that I'm done (there may be opportunities to move the parsing to its own module). But I do want something in place as a contingency. There's no need to rush it into production; I plan to leave this in draft status for a while, at least until after release 3.0.0.
|
Decent result with respect to fixer. Recent attempts have taken 1:18 to 1:27 to complete; this one completed in 0:50. |
One semi-valid message among a slew of false positives.
|
No concerns with Scrutinizer "complexity" warnings. |
|
Experiment with Calculation resulted in only 1 second of additional savings. Per plan, I will back it out of this ticket. |
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Aug 12, 2024
I am becoming concerned with the increasing run-time of php-cs-fixer, especially since it can time out. Some relief may come from PR PHPOffice#4118, but that won't be merged for some time, if ever. So, bump up the timeout period now. Also replace properties which php-cs-fixer has deprecated with their non-deprecated equivalents. No change to any source code.
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have been having some time-out problems with php-cs-fixer in my environment (no problems yet in Github). Bringing that up with them, the first thing they suggested was that it might be due to very large modules. The largest module they use in testing is a bit over 1,000 lines, and we have about 16 that exceed that. The biggest of these is Xls Reader; at 7,647 lines, it is more than 2,000 lines longer than its nearest competitor (Calculation), and at least 5 times larger than fixer's max.
It's not clear to me that breaking it up will actually solve my problem. On the other hand, perhaps it is time to do some re-factoring anyhow; as an example, changing the parsing of tfunc/tfuncv values from enormous select statements to indexing constant arrays, possibly external, is something that ought to have happened long ago. This turned out to be easier than I had thought. Breaking it into sub-modules each of which can access each other's protected properties and methods was fairly straightforward. There's a bit of overhead in having to allocate new classes, but Xlsx Reader has been doing that all along (without the protected access part).
I've managed to remove about 2,900 lines from Reader/Xls, scattering those among 2 existing and 6 new source modules. I'm not sure I've chosen the best possible, or most maintainable, approach. I'm not sure that I'm done (there may be opportunities to move the parsing to its own module). But I do want something in place as a contingency. There's no need to rush it into production; I plan to leave this in draft status for a while, at least until after release 3.0.0.
This is:
Checklist:
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.