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.

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

Refactor Config #182

Merged
merged 10 commits into from
Dec 29, 2024
Merged

Refactor Config #182

merged 10 commits into from
Dec 29, 2024

Conversation

haszi
Copy link
Contributor

@haszi haszi commented Dec 22, 2024

This PR refactors the Config class from using an array of options and method calls to access these to mostly using object properties. Probably best reviewed commit by commit.

haszi added 9 commits December 22, 2024 20:03
Remove methods mapping method calls to property access
Remove superfluous exportable() method
Add getter for color output
Remove superfluous set_color_output() method
Add code related to color output on windows to the constructor
Make copyright a readonly property that is initialized in the constructor
Remove copyright() method
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.

MSTM, I think it would be better to use camelCase for the property names, but this can wait.

Comment on lines 8 to 9
/** @var array<string, string> */
public array $output_format = [];
Copy link
Member

Choose a reason for hiding this comment

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

Is the change from static to instance properties okay? (not that I think having it static in the first place made a lot of sense).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes sense that one set of configuration options can be assigned to one object of the class Config and then we just pass the appropriate object when needed.

As far as how Config is used in PhD, this doesn't make much of a difference as only one Config object is used throughout. :-)

@haszi
Copy link
Contributor Author

haszi commented Dec 29, 2024

MSTM, I think it would be better to use camelCase for the property names, but this can wait.

I wanted to do this eventually but it makes more sense to do this as part of this PR.

@haszi haszi requested a review from Girgias December 29, 2024 12:15
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.

Let me know if you are happy as if for me to merge it as a rebase or if you want to stilll organise the commits.

@haszi
Copy link
Contributor Author

haszi commented Dec 29, 2024

Let me know if you are happy as if for me to merge it as a rebase or if you want to stilll organise the commits.

I'll leave it up to you to merge it as you want.

@Girgias Girgias merged commit b80ae36 into php:master Dec 29, 2024
7 checks passed
@haszi haszi deleted the Refactor-Config branch December 30, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants