Refactor Options Handler, Interface and Parser #107
Merged
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
added 6 commits
March 1, 2024 15:12
Refactor Options_Parser to require all dependencies as constructor parameters. Inject dependencies in render.php. Add type hints for injected dependencies in Options_Parser. Add method to check whether every element of the injected array is implementing Options_Interface.
Add type hints for methods that need them. Refactor array declaration from array() to []. Rename one method to better show its function.
Add parameter and return types to all methods. Make void returns explicit in methods that call other methods. Change array declaration from array() to []. Make code style consistent.
Fix parameter types. Refactor handlerForOption method to always return an array. Use strict equality operator for strings. Cast option key passed to handlers to string. Add short options from package handlers to short option list.
|
Smoke test failures for PHP 7.1-7.4 are due to parameter and return types. Ie. the minimum PHP version needs a bump to 8.0+. This need is not introduced by this PR though as the codebase already makes use of PHP 8.0+ features (e.g.: |
Girgias
reviewed
Mar 3, 2024
Remove string casts. Change the package handlers array of the Parser constructor to a variadic Options_Interface and remove validation method.
Girgias
approved these changes
Mar 5, 2024
|
Do you want to rebase the last commit into one of the prior commits for me to rebase, or just squash-merge the whole thing? |
|
I think a squash merge makes sense here. The only reason there are so many commits in this PR was for me to keep track of what I was doing (and possibly to make reviewing it easier). I'll squash commits like these in the future before I open a PR. |
|
You can keep the multiple commits when doing PRs as it makes reviewing easier :) |
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.
Most of this PR adds dependencies of
Options_Parseras constructor parameters (and injects them fromrender.php) and adds parameter and return types toOptions_Handler,Options_InterfaceandOptions_Parser.There are six smaller commits in this PR and it's probably easier to review it commit by commit.