Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor Options Parser and default Options Handler #110

Merged
merged 5 commits into from
Mar 17, 2024
Merged

Refactor Options Parser and default Options Handler #110

merged 5 commits into from
Mar 17, 2024

Conversation

haszi
Copy link
Contributor

@haszi haszi commented Mar 14, 2024

This PR includes the followings:

  • remove all direct setting of Config from the default Options_Handler and return an array of parsed options instead. Add return type declarations to each method returning an array
  • inject dependencies through Options_Handler's constructor in render.php
  • refactor a few files to allow them to be used from tests
  • add tests for the default Options_Handler and Options_Parser
  • fix inconsistent short and long option required/optional flags in Options_Handler

haszi added 5 commits March 14, 2024 17:21
Refactor Options Parser and default Options Handler
c54f885 
Refactor default Options Handler to accept dependencies through its constructor.
Refactor default Options Handler to return an array of options instead of directly setting options in Config and declare array return types.
Refactor method in Options Parser to return an array of options and declare its return type as array.
Inject all necessary dependencies in render.php.
Refactor two methods in default Options Handler by using match expressions.
Minor refactor of files used in tests
5a085c0 
Inlcude Config with require_once instead of require in Autloader.
Define __INSTALLDIR__ only if it is not defined yet in render.php.
Include Autoloader and functions.php with require_once instead of require.
Remove unnecessary __PHPDIR__ constant, correct path for __INSTALLDIR__ and use that constant in setup.php.
Remove var_dump from Options Parser
6422732
Fix inconsistent option flags in default Option Handler
462d8dc
Add default Options Handler tests
3ac956b
@haszi
Copy link
Contributor Author

haszi commented Mar 14, 2024

Possible future improvements of this part of Phd:

  • add a handle() method to Options_Interface (or a new Options_Handler interface) and implement it in the default Options_Handler class and simply pass CLI arguments/values to it
  • refactor CLI options parsing so that these can be passed in through GET/POST parameter values as well
  • refactor the default Options_Handler's short option/long option/method mapping (not sure what this would look like)
  • refactor the default Options_Handler so that it doesn't need an instance of Config and/or Format_Factory (not sure how this would work with all the settings and format info needed by the handler)

Girgias
Girgias approved these changes Mar 17, 2024
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Girgias Girgias merged commit e6e6847 into php:master Mar 17, 2024
8 checks passed
@haszi haszi deleted the Refactor-Options-Parser-and-default-Options-Handler branch March 18, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@haszi @Girgias