-
Notifications
You must be signed in to change notification settings - Fork 1
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
Pivotal ID # 185392832: Submission FTP files source #741
Pivotal ID # 185392832: Submission FTP files source #741
Conversation
this.sources.clear() | ||
return this.apply { builderAction() }.build() | ||
} | ||
fun buildFilesSourceList(builderAction: FilesSourceListBuilder.() -> Unit): FileSourcesList { |
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.
refactor to favor encapsulation.
@@ -15,7 +15,7 @@ class RegisterRequest( | |||
var path: String? = null, | |||
var orcid: String? = null, | |||
val notificationsEnabled: Boolean = false, | |||
val userFolderType: String? = null, | |||
val storageMode: String? = null, |
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.
changed to be consistent with submission attribute
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.
LGTM. Please consider my comments
return findFile(filePath)?.let { createFile(filePath, it, attributes) } | ||
} | ||
|
||
private fun findFile(filePath: String): File? { |
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.
As discussed, this could be changed in favor of a new implementation of ExtFile
that wouldn't require an instance of File
and would just contain the file's metadata
else -> path | ||
} | ||
|
||
val filePath = if (type == DIRECTORY_TYPE.value) path.removeSuffix(".zip") else 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.
👍
line() | ||
|
||
line("Study") | ||
line("File List", "FileList.tsv") |
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.
Since this test is specifically for FTP files, I would also include an inner submission file and not only file lists files, just to test all the branches of the code
…services into feature/pivotal-185392832-Submission-FTP-files-source
…services into feature/pivotal-185392832-Submission-FTP-files-source # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
https://www.pivotaltracker.com/story/show/185392832