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

Autoload generator refactor #2563

Closed

Conversation

donquixote
Copy link
Contributor

Don't pull this yet, this is in review / demo stage.
I am going to rebase this quite a lot in the next days.

See Issue #2460: AutoloadGenerator refactoring - strategy talk

Known issues:

  • Names might not be ideal. Especially "PluginInterface" I feel should rather be something else. This is not about providing an API, it is only to organize our internal code better. Please be constructive and suggest alternative identifiers!

Fixed issues:

  • Some of the changes should rather go into a separate PR. Especially those about AutoloadGeneratorTest.
    Solution: This was now factored out into Autoload generator test improvements #2666, and merged into master.
  • Commits are just "commit everything I have at a time", I will rebase when it has shaped up.
    Solution: Not it is all squashed into one commit.

'psr-0' => array('A\\B' => $this->workingDir.'/lib'),
'classmap' => array($this->workingDir.'/src')
));
$mainPackage->setRequires(array(new Link('z', 'a/a')));
Copy link
Member

Choose a reason for hiding this comment

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

Please refrain from doing ANY changes to tests in this PR. Send them in a separate PR if you must, so they can be checked against the existing code.

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 will isolate these changes in the next or an upcoming rebase.
(I think they are already speparate commits, but I will make an isolated PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Factored out into #2666

donquixote added a commit to donquixote/composer that referenced this pull request Jan 9, 2014
donquixote added a commit to donquixote/composer that referenced this pull request Jan 10, 2014
donquixote added a commit to donquixote/composer that referenced this pull request Feb 4, 2014
donquixote added a commit to donquixote/composer that referenced this pull request Feb 4, 2014
donquixote added a commit to donquixote/composer that referenced this pull request Feb 4, 2014
donquixote added a commit to donquixote/composer that referenced this pull request Feb 4, 2014
@donquixote
Copy link
Contributor Author

The testing stuff is now isolated into a separate commit, and merged into master.
The remaining commits are squashed.
I am waiting for new reviews!

*
* @return PluginInterface[]
*/
protected function createPreparedPlugins(PackageMap $packageMap, $scanPsr0Packages)
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming of this method is confusing

*/
public function addArraySourceFile($filename, $phpRows)
{
$this->arraySourceFiles[$filename] = $phpRows;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add (string) casts here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure - would want a 3rd opinion.
Imo, if this method is called with something other than a string, then something is going wrong, and i want to see it crash loud and clear, instead of a silent type cast. Or we simply let the IDE do the type checks for us. But that's just me.
@Others?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really care for string casts. It's just noise IMO especially since it'll be automatically casted to a string when it's used as such later.

@Seldaek
Copy link
Member

Seldaek commented Feb 2, 2020

Think it's probably time to close this..

@Seldaek Seldaek closed this Feb 2, 2020
@Seldaek Seldaek modified the milestones: 2.x, 2.0-core Feb 12, 2020
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

5 participants