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

Return one Autoloader per namespace #63

Closed
wants to merge 4 commits into from

Conversation

BrianHenryIE
Copy link
Contributor

When a composer.json's autoload key contains two namespaces, return two Autoloader objects. e.g. RubixML/Tensor

https://github.com/RubixML/Tensor/blob/86bb628781f311684a6778744a22b283efdae58f/composer.json#L36-L39

When a composer.json's autoload key contains two namespaces, return two Autoloader objects. e.g. RubixML/Tensor

https://github.com/RubixML/Tensor/blob/86bb628781f311684a6778744a22b283efdae58f/composer.json#L36-L39
@BrianHenryIE BrianHenryIE marked this pull request as draft September 3, 2020 04:06
@coenjacobs
Copy link
Owner

@BrianHenryIE Can I help you proceed with this PR? Is it ready to review yet or do you need anything?

@BrianHenryIE
Copy link
Contributor Author

It's definitely not ready, TBH I never even ran the unit tests. The main point in the draft PR was to share the identified problem.

I just personally encountered the un-prefixed classes (#64, #58) so I'll be taking a look at that soon and I think as I fix it it might make sense to polish this PR too.

@BrianHenryIE
Copy link
Contributor Author

BrianHenryIE commented Feb 13, 2021

This is pretty much ready to merge. I expect after the other PRs are merged, this will need master merged again first.

It changes Autoloader classes from using a constructor to using a static method which will return an array of Autoloader[], so both will be returned in the example:

"autoload": {
        "psr-4": {
            "Tensor\\": "src/",
            "JAMA\\": "lib/JAMA"
        }
    },

I wrote tests for Composer/Package.php (although only one was a failing test for this PR).

@BrianHenryIE
Copy link
Contributor Author

I removed getSearchNamespace() from Composer/Autoload/Classmap.php and from the Autoloader interface because it seems it's already more appropriately part of NamespaceAutoloader abstract class (which implements Autoloader).

That's what the Psalm warning is about. TBH, I only ran Psalm on the files that have been changed. As I said above, I expect to have to merge master in again before before this gets merged itself, and I know there are Psalm fixes already written that will conflict / waste effort if I were to write them here.

@darthvader666uk
Copy link

Thought I will give up an Update.

Been testing my code with this pull (via the issues that was raised: #62) and so far so good. AWS is doing what I want it to do (though It wont do it in the vendor folder directly, it needs to be done outside of it).

Will keep you posted and update more findings but so far, so good

@BrianHenryIE BrianHenryIE deleted the issue-48 branch May 13, 2021 06:07
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