Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Feature] Add xquery recursive collection creation with just one path expression #5062
base: develop
Are you sure you want to change the base?
[Feature] Add xquery recursive collection creation with just one path expression #5062
Changes from 3 commits
7ee7a8d
206772a
66a060e
f907b10
5135da7
78bf82e
e898899
31abe51
ff155b8
e7118ed
ac5f200
7cbd6dd
43f331f
cf1a8de
1915cfa
3ed9193
213d9bd
11b270b
138b435
fa16fd4
61faaae
72bc0ee
bd4d1d2
b963c00
4f71bdc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How about:
Differences:
collectionName
is improved by beingfinal
.if(
args.length
is on the LHS side of the comparison, this is the most common formulation throughout the code base.else
instead of theelse if
means thatcollectionName
can never benull
, and therefore cannot potentially cause an NPE elsewhere.However it strikes me that when using the 1 argument version of the function, then
collectionName
is perhaps a bit of a misleading name, as for that function it will hold the collection path.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.
shoudl this go in to
try(final Collection newCollection..)
?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.
I am not certain, but the code formatting whitespace here looks a bit mangled to me.
Check warning on line 105 in exist-core/src/main/java/org/exist/xquery/functions/xmldb/XMLDBCreateCollection.java
Codacy Production / Codacy Static Code Analysis
exist-core/src/main/java/org/exist/xquery/functions/xmldb/XMLDBCreateCollection.java#L105
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.
Would it be possible to use an ARM (Automatic Resource Management) approach to handle collection closure through utilising Java's
try-with-resources
statement instead?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.
I don't think we need log this.
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.
I don't thin we need log this.
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 should be trace level instead
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 should be
trace
level insteadThere 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.
exception should be marked
final
Check warning on line 156 in exist-core/src/main/java/org/exist/xquery/functions/xmldb/XMLDBCreateCollection.java
Codacy Production / Codacy Static Code Analysis
exist-core/src/main/java/org/exist/xquery/functions/xmldb/XMLDBCreateCollection.java#L156
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.
Avoid catch-all
Exception
. Instead specify explicitly multiple exceptions separated by|
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.
code has gone?
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.
Avoid
/db
, instead use existing constant from theXmldbURI
class.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.
Do I understand correctly that you are currently testing that:
creates the collection
/db/parent-collection/path/to/new-collection
?If so, that should not work! The
xmldb:create-collection#1
function should always take an absolute path, i.e. starting with/db
. This test should be modified to check for an error result, and the implementation updated accordingly.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.
looks good to me