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

Skip namespaces #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

leoloso
Copy link

@leoloso leoloso commented Feb 6, 2020

Fix for #27

@leoloso
Copy link
Author

leoloso commented Feb 6, 2020

Tests are not passing, so will fix the problem and open a new PR

@leoloso leoloso closed this Feb 6, 2020
@leoloso
Copy link
Author

leoloso commented Feb 6, 2020

Fixed

@leoloso leoloso reopened this Feb 6, 2020
@coenjacobs
Copy link
Owner

@leoloso As discussed on Twitter earlier today, sorry for not getting back to you sooner. Life sometimes just gets in the way. You correctly pointed out that I should have been more communicative about it and not leave you in the dark like this. There is no point from my side to continue discussing this over and over again, I dropped the ball and I promise to do better in the future. Let's get back to what this PR is about and why it hasn't been merged yet.

I like this new feature, but not the way it has been implemented. This basically changes the method signature of a lot of methods, where the parameter isn't used. It comes down to passing a variable through all the replacing methods, which is rarely used. I appreciate the effort to make the code more readable, but should we ever want to use another parameter like this, it just keeps on changing the method signature for each individual parameter.

Therefore I propose that I make a Config object that holds the Mozart configuration as read from the project composer.json file. That Config object can then be passed on to the various classes having a need for it and your logic can then use that Config object to check if your specific parameter is being used. Does that sound like a good idea, also bearing in mind that this will be used for the core configuration and more optional parameters (for example the newly introduced delete_vendor_directories and override_autoload, which are using their own unique approach as well)? I will write up a proposal for this Config object this weekend.

@leoloso
Copy link
Author

leoloso commented May 23, 2020

I have no objections to your proposed changes :) All I care about is the functionality, not how it is implemented. So please go ahead and reject this PR.

My use case is this one: my project has a few dozens components under namespace "PoP" (PoP\Engine, PoP\Root, etc) and many others from everyone else (Symfony, etc). All the "PoP" ones need not be scoped, so I want to add a rule that says: skip everything starting with "PoP"

@XedinUnknown
Copy link

Instead of a config object, why not simply use a DI container as a single source of truth for the whole application?

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.

3 participants