Details
- Reviewers
epriestley richardvanvelzen - Group Reviewers
Blessed Reviewers - Commits
- rPHU44113cfd0db1: Add an `XHPAST::getNamespace()` method
Added unit tests.
Diff Detail
- Repository
- rPHU libphutil
- Branch
- master
- Lint
Lint Passed Severity Location Code Message Advice src/parser/xhpast/api/XHPASTNode.php:260 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 8913 Build 10444: Run Core Tests Build 10443: arc lint + arc unit
Event Timeline
I'd like to test this on some more complex input data, but I think that my logic is correct for basic cases.
The test data format is a bit hacky and I'm open to suggestions.
| src/parser/xhpast/api/XHPASTNode.php | ||
|---|---|---|
| 260 | Err, this can be removed now :) | |
Maybe simpler is:
- Get all n_NAMESPACE from the tree.
- Reverse them.
- For each namespace:
- If the namespace is after the node (its first namespace token is after the node's last token), continue to the next namespace (the declaration is later in the file).
- If the namespace has a body but the node is not in the body (the node's last token is not in the namespace's token range), continue to the next namespace.
- Return the namespace (this is the preceding body-free namespace, or a bodied namespace containing the node).
- Return null if you get this far.
I think your implementation is also fine, it's maybe just a little harder to reason about, especially with the exit condition being shifting null off the list. I think we end up with a huge pile of nulls in $nodes? Might be a little unintuitive if this gets touched in the future. But there's test coverage so this isn't really perilous or anything.
| support/xhpast/xhpast.cpp | ||
|---|---|---|
| 15 | Haha, this makes sense but let's use 5.8.1 just so we aren't traveling back in time? | |
Yeah actually I think that I've coverage almost everything except for this:
<?php namespace X { namespace Y { class Z {} } }
But this isn't valid PHP anyway (although XHPAST parses it fine).
I also like this better than D14506 for solving the problem, PHP's grammar makes this not a particularly great fit for solving via AST construction.
The new method is somewhat more complex, so I'd appreciate another look at it. I think that the added complexity is okay because it is in the form of new methods (AASTNode::isBefore(), AASTNode::isAfter() and AASTNode::containsDescendant()), which should be more useful generally.
Seems good to me -- those pieces seem reasonable and I think the logic is a little easier to understand now with them broken out.
| src/parser/aast/api/AASTNode.php | ||
|---|---|---|
| 44–52 ↗ | (On Diff #35105) | Or $this->getTree()->getRootNode() or whatever? |
| src/parser/xhpast/api/XHPASTNode.php | ||
|---|---|---|
| 291 | We generally seem to avoid static methods... should I remove static here? | |
| src/parser/xhpast/api/XHPASTNode.php | ||
|---|---|---|
| 273 | You probably meant $namespace->getNamespaceName(). getNamespaceName doesn't accept any parameters. | |