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
Refactor Resource interface #5422
Refactor Resource interface #5422
Conversation
FileUtil.appendName()
…fb_resource_refactor
Did this break the FileContent tests? |
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 looks good, with the possible exception of the FileContent test failure.
return new Path(subDirs); | ||
// TODO: Schema names can contains slashes (e.g. assay names) | ||
// TODO: Not sure how the callers deal with this, when they map these into file-system directories. | ||
// TODO: We could throw IllegalArgument and let the caller handle it. |
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'd vote for replacing /
with_
. If we throw a runtime exception, it's unlikely that callers will know they may need to deal with it.
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 didn't want to break things in release branch.
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.
Should we add a warning on callers of new File(File, String)
?
Yes, on develop |
Hmm, passed several times... Let's see what teamcity says... |
The sendDigest functionality seems to be sensitive to database/webserver time mismatch. |
Rationale
Remove ambiguous methods in interface Resource.
Prefer Path and Path.Part to ambiguous usage of String.
Related Pull Requests
Changes
Path.getPathPart()
FileUtil.appendName()