-
Notifications
You must be signed in to change notification settings - Fork 29
Added method to get a nodes localname #130
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
Conversation
looks ok. what is the use case? i am wondering a bit if we should not expect a node name instead of a path... or we could say it can be either and first check if there is a |
the use case may or not be a valid one, in jackalope-fs I want to index the localname - but as I already index the path I'm not sure thats necessary, but I am still learning what is and is not possible with Zend Lucene. |
for jackalope, the node name should usually be including the namespace i guess. but i think if we can support both path and just a full name where we only have to remove the namespace, i am +1 on having this. might be we actually do such splits in jackalope for some reason already. |
do you think you manage to support both path and just a name in one method? |
ping .. guess it would be good to resolve this for 1.2.0 stable |
what i mean by my comment: could we make this method work with both absolute paths and full names as parameter, to return the local name regardless of what was the argument? we could maybe just strrpos from the end of the string for either : or / whichever is first, and the whole string if none, and take the last segment. then we have no validation if this is a valid name / path but would handle a lot without much effort. |
@dbu well accepting a node name would make the behavior different from |
return array( | ||
array('/parent/child', 'child'), | ||
array('/parent/ns:child', 'child'), | ||
array('/parent/ns:child:foo', 'child:foo'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an invalid name. i would not have a test for this situation. (if anything, this should lead to an exception)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
@dantleech okay. if the local name method makes sense for you as it is, lets keep it that way then. i added a new comment for the test with an invalid node name. |
so good to merge so that we can tag a stable release? |
9d724f1
to
4b7003b
Compare
Have removed the invalid test - not sure it should be the responsiblity of that helper method to validate the node name, in anycase getNodeName does not. |
Added method to get a nodes localname
Added method to retrieve the localname for a node from a given path.