Skip to content

Conversation

@anchela
Copy link
Contributor

@anchela anchela commented Jun 28, 2022

@joerghoh , @reschke , @mreutegg , would you have time to take a look this improvement? thanks

@anchela anchela requested review from mreutegg and reschke June 28, 2022 18:16
Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that equivalent methods already exist in JackrabbitSession. Maybe it would be better to just delegate there. In which case we might be able to do all of this in a default impl in the interface...

I'm also not convinced that getNodeOrNull can return null although the node exists (but is not a JackrabbitNode); maybe it would be better to change the return value to a "regular" node?

@anchela
Copy link
Contributor Author

anchela commented Jun 29, 2022

hi @reschke , thanks for looking into the PR.

Note that equivalent methods already exist in JackrabbitSession. Maybe it would be better to just delegate there.

not quite. JackrabbitSession.getNodeOrNull takes an absolute path where as Node.getNode and Node.getNodeOrNull take a relative path. So, i don't think we can delegate it there.

Regarding:

I'm also not convinced that getNodeOrNull can return null although the node exists (but is not a JackrabbitNode);
maybe it would be better to change the return value to a "regular" node?

if the node exists but is not accessible it would also return null.
in the first draft, i had the method return a Node but then I wondered if there is any reasonable scenario where a Node is a JackrabbitNode but one of it's child nodes isn't..... and i concluded that this is very unlikely and thus return null instead of having code that might through classcasteexception was fine for the default implementation.
but i don't have a strong preference.... return just a Node would likely require API consumers to cast it to JackrabbitNode again or do some extra check.... but as I said... no strong preference.

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All ok; I misread the diffs and did not realize I was indeed looking at the default impl in the interface.

@anchela
Copy link
Contributor Author

anchela commented Jun 30, 2022

@mreutegg , unless you have any concerns i would go ahead a merge the improvement.

Copy link
Contributor

@mreutegg mreutegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NodeImpl is currently defined as implements Node, JackrabbitNode. I think this could be simplified into just implements JackrabbitNode.

@anchela
Copy link
Contributor Author

anchela commented Jun 30, 2022

@mreutegg , good catch! i fixed it.

@anchela anchela requested a review from joerghoh July 1, 2022 11:04
@anchela
Copy link
Contributor Author

anchela commented Jul 1, 2022

@joerghoh , just wanted to see if i can invite you for reviews now.... :-) works. please approve or request changes.... or merge to give it a try....

@joerghoh joerghoh merged commit 0adc162 into trunk Jul 1, 2022
@joerghoh joerghoh deleted the OAK-9819 branch July 6, 2022 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants