35 audit one liners [pr] #36
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
I might suggest logging this and the following error at the WARN or INFO level as this appears to default to a value when the input is not correct.
An alternative syntax for map get is ids[key]. See https://groovy-lang.org/style-guide.html#_native_syntax_for_data_structures https://stackoverflow.com/questions/14196454/groovy-safely-find-a-key-in-a-map-and-return-its-value
Typically, the fields and constructor would appear at the top of the class. These should also be camelCase. For reference: https://stackoverflow.com/a/12427127
A common naming convention for constants like this string would be upper snake case like INSTALL_PREFIX https://google.github.io/styleguide/javaguide.html#s5.2.4-constant-names
> Should this be logged at the ERROR level? Not really. We can use this same codepath when creating a new package.
The safe navigation operator seems unnecessary here due to the preceding !query check.
I think this would be more readable to pass the closure directly
.warning -> .warn
QuiltObserverFactory.groovy: 37: [Static type checking] - Incompatible generic argument types. Cannot assign java.util.List <nextflow.quilt.QuiltObserver> to: java.util.Collection <TraceObserver>
@ line 37, column 9.
[new QuiltObserver()]
The Groovy safe navigation operator is a nice way to express this:
Groovy also has an exclusive range operator: beginIndex..<endIndex.
You should be able to omit the package name here:
to Groovy truthiness, empty string and null are both false
@unroll is not needed here.
@unroll is not needed here.
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network because the title contains "[pr]".
Check the status or cancel PullRequest code review here.
|
NOTE: The 'Type of change' is slightly misleading. These changes are purely stylistic, and should not have any functional impact. |
There was a problem hiding this comment.
I've gone through the attached audit and the list of fixes provided and confirmed that the list in the PR descriptions have been resolved. I left one clarifying comment about PRComment_033, but it is non blocking.
Reviewed with ❤️ by PullRequest
plugins/nf-quilt/src/main/nextflow/quilt/nio/QuiltFileSystem.groovy
Outdated
Show resolved
Hide resolved
PRComment_033, but it is non blocking.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
First steps for #34
Identifier are from PullRequest.com report
https://github.com/quiltdata/nf-quilt/files/10470527/reportgen.zip
PRComment_002
PRComment_005
PRComment_009
PRComment_010a
PRComment_012
PRComment_015a
PRComment_016
PRComment_017
PRComment_018 [upper snake case]
PRComment_021a
PRComment_027
PRComment_030
PRComment_031
PRComment_032
PRComment_033
PRComment_036
PRComment_038
PRComment_040