Make Context leaner and enable third parties to create them #53
Conversation
Contexts are necessary for factories to resolve QSEN into FQSENs based on partial namespaces and namespace aliases. These provide DocBlocks with the namespace name and namespace aliases. The new ContextFactory will enable third parties who don't use phpDocumentor's Reflection component to construct a Context based on a class reflector or namespace name (and file contents).
|
@jaapio can you review this item? |
| */ | ||
| public function __construct($namespace, array $namespaceAliases = []) | ||
| { | ||
| $this->namespace = ('global' !== $namespace && 'default' !== $namespace) |
There was a problem hiding this comment.
global and default should be class consts, But I don't get it yet why global and default are not allowed as namespace?
There was a problem hiding this comment.
Backwards compatibility with parts of phpDocumentor that have been there for a long time; I cannot predict what will break if I remove them. Perhaps after we have cleaned up the rest of the app?
Make Context leaner and enable third parties to create them
|
This looks good! A little note: the code I used from Doctrine that parsed the class didn't read the whole file. I expect this to make a noticeable difference (reading whole file + I wonder though if their solution works with multi-namespace files. |
|
@mnapoli I already found a bug in their solution that was not optimal :) |
|
ok great :) |
|
@mnapoli also: i will be profiling this package before release; for phpDocumentor it is equally important that this is fast ;) |
|
Awesome! |
|
Why is the Context class moved from Also, the tests are cleary failing:
The same goes for my package using this library, class Context can't be found.. Could you please fix this issue? |
| * @param string $namespace The namespace where this DocBlock resides in. | ||
| * @param array $namespaceAliases List of namespace aliases => Fully Qualified Namespace. | ||
| */ | ||
| public function __construct($namespace, array $namespaceAliases = []) |
There was a problem hiding this comment.
Why is $namespace required? This is a BC break, the previous default was ''
There was a problem hiding this comment.
And the short array syntax isn't supported in 5.3. So if this was intentional, bump the composer.json requirements.
|
@barryvdh The changes to the composer.json are on my machine; I didn't want to submit a huge PR :) Also: you might want to fix your package to a tag instead of dev-master; we are working towards version 2 of this component which will have significant BC breaks |
|
In addition to the BC breaks might master have some instability in the next two weeks until the refactoring is complete |
|
@mvriel I have In my specific case, I do require a tag, but people are sometimes using a global dev-stability, eg. for testing Laravel 5.1 a few weeks before stable (with dev Symfony versions etc.) If you plan to break BC, could you please update your branch-alias to 3.0.x, or use a seperate branch for the 3.0 changes? |
|
@barryvdh my apologies; I was mistaken with another component (I was deleting my comments because I just found out) |
|
@barryvdh sorry about the mix up; I have changed the branch-alias of dev-master to 3.0 as you suggested. If all is well that should fix any issues that you experience? |
|
Yeah I guess so :) Thanks! |
Make Context leaner and enable third parties to create them
Contexts are necessary for factories to resolve QSEN into FQSENs based
on partial namespaces and namespace aliases. These provide DocBlocks
with the namespace name and namespace aliases.
The new ContextFactory will enable third parties who don't use
phpDocumentor's Reflection component to construct a Context based on
a class reflector or namespace name (and file contents).