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

Config value object instead of manually reading the file #47

Merged
merged 29 commits into from
Sep 11, 2024

Conversation

coenjacobs
Copy link
Owner

@coenjacobs coenjacobs commented May 23, 2020

I'd probably want to leave this out of 0.6.0, but haven't decided or full tested yet.

@BrianHenryIE
Copy link
Contributor

BrianHenryIE commented Feb 8, 2021

I think a config object is a very good idea – it gives a good place for input sanitisation (e.g. I think relative directory paths should always have no leading slash and should have a trailing slash), defaults, and documentation. I think unit/integration tests could be tidied up with a subclass with sensible defaults which could be extended.

Your stringly typed approach misses out on a lot of benefits that could be gained with explicit getters and setters. And I'm hoping with the project PHP update and Pslam, you agree.

I've built a class with all the composer/extra/mozart options as properties. I'm very much in draft status, but for discussion:

I've been playing around with JsonMapper/JsonMapper but now I think cweiske/jsonmapper is worth a look – both for how Package.php could parse a composer.json into its typed class.

I've also looked at Composer's own Factory::create() to parse composer.json. It doesn't immediately show a huge benefit (from debugging in PhpStorm), but I'm thinking there might be benefits where e.g. the autoload key has used array rather than object (vice versa), or e.g. returns single/multiple maps like #63. What is valid for Composer is all that needs to be valid for Mozart so what has been parsed by them is accurately what Mozart should consider.

I was also looking at Composer's Factory::create() to see if it could help my #38 issue (mentioned just recently in #66) for delete_vendor_directories to use to safeguard against deleting directories with changes made – Composer itself warns me when I run composer install if I have modified any files.

@BrianHenryIE BrianHenryIE mentioned this pull request Mar 13, 2021
szepeviktor pushed a commit to szepeviktor/BrianHenryIE_mozart that referenced this pull request Jan 9, 2023
szepeviktor pushed a commit to szepeviktor/BrianHenryIE_mozart that referenced this pull request Jan 9, 2023
@coenjacobs
Copy link
Owner Author

Thank you @BrianHenryIE, even though we have parted ways on this project, for being a help in getting this done. I think Mozart is in a better place with this Config object in place, it sparked a ton of improvements and more test coverage.

@coenjacobs coenjacobs marked this pull request as ready for review September 11, 2024 15:27
@coenjacobs
Copy link
Owner Author

This can probably be improved a lot still, but I'd rather consider this a starting point for getting the next stable version out the door. This makes testing and also maintenance a lot easier to do. So I'm merging this in and will start preparing a release around it.

@coenjacobs coenjacobs merged commit fb98a64 into master Sep 11, 2024
3 checks passed
@coenjacobs coenjacobs deleted the config-value-object branch September 11, 2024 15:28
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