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

Path based access to datanode children #343

Merged
merged 8 commits into from May 28, 2019

Conversation

Projects
None yet
2 participants
@tsiq-karold
Copy link
Collaborator

commented May 23, 2019

closes #282

@tsiq-karold tsiq-karold requested a review from MykolaGolubyev May 23, 2019

@tsiq-karold

This comment has been minimized.

Copy link
Collaborator Author

commented May 23, 2019

@MykolaGolubyev, I haven't updated documentation yet. I was initially thinking of putting it in https://twosigma.github.io/webtau/guide/REST/data-node but it's really Java specific. I mean it would work in Groovy but it doesn't make sense to use it in Groovy. The alternative is Junit pages. WDYT?

@MykolaGolubyev

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

I think that even in Groovy case it can be useful if someone decides to get/build a path at runtime. Or have a path inside a config file (i.e. different versions of api to access temperature from a city).

int additionalOpenIdx = name.indexOf('[', openBraceIdx + 1);
int additionalCloseId = name.indexOf(']', closeBraceIdx + 1);

if (additionalOpenIdx != -1 || additionalCloseId != -1 || openBraceIdx > closeBraceIdx) {

This comment has been minimized.

Copy link
@MykolaGolubyev

MykolaGolubyev May 23, 2019

Contributor

what about cases like [[1]

This comment has been minimized.

Copy link
@tsiq-karold

tsiq-karold May 23, 2019

Author Collaborator

That'll throw. That's what the additional*Idx checks are for, they look for brackets after the first one.

This comment has been minimized.

Copy link
@MykolaGolubyev

String indexStr = name.substring(openBraceIdx + 1, closeBraceIdx);
try {
int idx = Integer.valueOf(indexStr);

This comment has been minimized.

Copy link
@MykolaGolubyev

MykolaGolubyev May 23, 2019

Contributor

we either have a util for conversion from a string to number avoiding try-catch or we should add

This comment has been minimized.

Copy link
@tsiq-karold

tsiq-karold May 23, 2019

Author Collaborator

OK, I'll check

This comment has been minimized.

Copy link
@tsiq-karold

tsiq-karold May 23, 2019

Author Collaborator

Actually, we don't really need the try-catch here as such, NumberFormatException is a runtime exception. I just wanted a more useful error message which is why I catch it and throw something different.

This comment has been minimized.

Copy link
@MykolaGolubyev

MykolaGolubyev May 24, 2019

Contributor

I see, I would still extract it somewhere locally.

Show resolved Hide resolved ...t/groovy/com/twosigma/webtau/http/datanode/StructuredDataNodeTest.groovy
String firstName = node.get('key1[0].name').get()
firstName.should == 'name1'

String lastName = node.get('key1[-1].name').get()

This comment has been minimized.

Copy link
@MykolaGolubyev
@tsiq-karold

This comment has been minimized.

Copy link
Collaborator Author

commented May 23, 2019

I think that even in Groovy case it can be useful if someone decides to get/build a path at runtime. Or have a path inside a config file (i.e. different versions of api to access temperature from a city).

That is true, I guess there are Groovy usecases because I think its value to Java users is much more significant. I also need it for #221 .

public boolean has(String name) {
return children.containsKey(name);
public boolean has(String pathOrName) {
return !(get(pathOrName) instanceof NullDataNode);

This comment has been minimized.

Copy link
@tsiq-karold

tsiq-karold May 23, 2019

Author Collaborator

@MykolaGolubyev I wasn't massively keen on this approach. One possible small improvement would be to add an isNull on DataNode instead of checking the type.

The alternative would require a workflow very similar to the get functionality but instead of doing the ultimate children.get, it would do children.containsKey so it's actually no better in terms of computational effort and would either require lots of dupe code or some ugly parameterisation.

This comment has been minimized.

Copy link
@MykolaGolubyev

MykolaGolubyev May 23, 2019

Contributor

isNull sounds reasonable. Adding default method should be backward compatible right?

This comment has been minimized.

Copy link
@tsiq-karold

tsiq-karold May 23, 2019

Author Collaborator

Yes.

@tsiq-karold

This comment has been minimized.

Copy link
Collaborator Author

commented May 23, 2019

Created #344 to track documentation.

@MykolaGolubyev
Copy link
Contributor

left a comment

I left one comment, but up to you.

if (isList()) {
return getAsCollectFromList(name);
return getAsCollectFromList(nameOrPath);

This comment has been minimized.

Copy link
@MykolaGolubyev

MykolaGolubyev May 24, 2019

Contributor

now I got confused as to how this will work in case of path. getAsCollectFromList signature is also talking about name only

@MykolaGolubyev MykolaGolubyev self-requested a review May 24, 2019

@MykolaGolubyev

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

I left one comment, but up to you.

Not sure now how to un-approve. There is getAsCollectFromList that I need your help to understand how will it work in case of path not name

@tsiq-karold

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2019

I left one comment, but up to you.

Not sure now how to un-approve. There is getAsCollectFromList that I need your help to understand how will it work in case of path not name

Conceptually it hasn't really changed. It'll apply the name or path logic to each member of the list if you reference a list element without [].

@tsiq-karold tsiq-karold merged commit 1d66e2f into master May 28, 2019

4 checks passed

Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tsiq-karold tsiq-karold deleted the datanode-path-get branch May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.