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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

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
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.

None yet

2 participants