Fix: prevent $sourceFilePathNames from shadowing retroactively adde…
#430
Conversation
rela589n
force-pushed
the
fix-colocated-mapping-driver-iterable-files-shadowing-paths
branch
from
9a1346b to
2d6de3d
Compare
|
Please make sure the subject of your commit message are no longer than 50 chars, and please wrap the body of your commit messages at 72 chars. |
rela589n
force-pushed
the
fix-colocated-mapping-driver-iterable-files-shadowing-paths
branch
from
2d6de3d to
518bad4
Compare
Could you please check it right now? I've shortened the commit messages, but the last commit message is not quite up to par. If needed, I can try to shorten it even more, but it would lose the context then. |
|
Why do you say you would lose the context? Can't you move it from the commit message subject to the commit message body? You did read that I asked to wrap the commit message body at 72 chars? Wrap, not truncate. |
After github.com/doctrine/pull/429/ was merged, it could introduce subtle bugs during migration toward the new `$sourceFilePathNames` approach. If client code called `addPaths()` after instance creation, it would be ignored (without any warning), and paths would not be used. The current commit fixes that. Retroactively added paths will be taken into account along with the `$sourceFilePathNames` iterable.
When checking $includedFiles for an included file, the previous `in_array()` approach, executed in a loop, is very expensive. Basically, it results in performance of O(N * M), where N - number of declared classes, M - number of included classes. The current approach is O(N), since `isset()` check has constant `O(1)` time.
rela589n
force-pushed
the
fix-colocated-mapping-driver-iterable-files-shadowing-paths
branch
from
518bad4 to
d0e7922
Compare
Sorry, didn't get your point. |
|
Wrapping just means adding a newline/carriage return every 72 chars. |
|
Now that I am on desktop, it does look wrapped just fine 👍 So why do you say you would lose the context? Can't you move it from the commit message subject to the commit message body? |
Done |
This change makes it easy for the depending libraries to use iterable of `SplFileInfo`, adapted to the format of `ColocatedMappingDriver`. For example, one could pass in Symfony Finder, adapted with `PathNameIterator` into `AttributeDriver` without having to reinvent it in every particular case.
| * @template T | ||
| */ | ||
| private function pathNameIterator(iterable $filesIterator): Generator | ||
| private function mergeIterables(iterable $iterable1, iterable $iterable2): Generator |
There was a problem hiding this comment.
This is rather appending $iterable2 to $iterable1 than merging.
There was a problem hiding this comment.
Do you propose naming it as concatIterables()?
"Appending" sounds with the tone of modifying the iterable, while it's actually combining them into a single iterable, doing no modification to the original.
Dictionary-wise, "merge" looks good as it means "combine or cause to combine to form a single entity."
|
Closing in favor of a better incoming PR |
3 participants
This PR fixes a possible bug introduced after #429
If the client code had called
addPaths()after the instance creation, it would've been ignored (without any warning or so), and paths would not have been used. This commit fixes that. Retroactively added paths will be taken into account as well as$sourceFilePathNames.