Petab download and upload #6
Conversation
There was a problem hiding this comment.
Heya @wow-such-code ! Awesome work. Maybe you can showcase it to me later and address the NITs and questions i have just to ensure that i understood it correctly 👍
|
|
||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
|
||
| public class IdWithPattern { |
There was a problem hiding this comment.
If the id has a pattern does it make sense to check for the pattern during generation?
There was a problem hiding this comment.
once we know more about it, yes
| System.out.println(); | ||
| System.out.println("Reference datasets found, uploading dataset..."); | ||
| System.out.println(); | ||
| //TODO copy and remove source references here |
There was a problem hiding this comment.
You still have stuff "ToDo" ;)
| //TODO copy and remove source references here | |
| //TODO copy and remove source references here |
| //@Option(arity = "1..*", paramLabel = "<parent_datasets>", description = "Optional list of dataset codes to act" | ||
| // + " as parents for the upload. E.g. when this dataset has been generated using these datasets as input.", names = {"-pa", "--parents"}) |
There was a problem hiding this comment.
Why was this commented out? Can this be removed?
There was a problem hiding this comment.
the ids are now provided via the yaml file, but there might be feedback that this option is better
| while (true) { | ||
| String line = reader.readLine(); | ||
| if (line == null) { | ||
| break; | ||
| } | ||
| if(inIDBlock && line.contains(":")) { | ||
| inIDBlock = false; | ||
| } | ||
| if(inIDBlock) { | ||
| String[] tokens = line.split("-"); | ||
| if(tokens.length == 3) { | ||
| String datasetCode = tokens[1].strip()+"-"+tokens[2].strip(); | ||
| sourcePetabReferences.add(datasetCode); | ||
| } | ||
| } | ||
| if (line.contains("openBISSourceIds:")) { | ||
| inIDBlock = true; | ||
| } |
There was a problem hiding this comment.
I think this section would benefit greatly from a small refactor into distinct methods that outline why the steps are taken for each if clause. Additionally these methods can more easily specify what happens if the input is malformed (token length != 3, why is the inIdBlock set to true even though it could be previously set to false etc.)
| File directory = new File(dataPath); | ||
| List<String> sourcePetabReferences = new ArrayList<>(); | ||
|
|
||
| File yaml = findYaml(directory); |
There was a problem hiding this comment.
NIT: Maybe having a distinct exit condition makes sense? If a yaml was provided but is empty, this currently will return a PetabMetadata object with empty metadata, which could be confusing 🤔
There was a problem hiding this comment.
Currently only parent references are stored in the metadata and those can be empty, which is checked in the respective command.
| return; | ||
| } | ||
| DatasetWithProperties result = new DatasetWithProperties(datasets.get(0)); | ||
| Optional<String> patientID = openbis.findPropertyInSampleHierarchy("PATIENT_DKFZ_ID", |
There was a problem hiding this comment.
Can this be assumed as always present? Maybe it makes sense to move this into a constant at the top of the class?
There was a problem hiding this comment.
this is currently not used, we need more feedback first
| DatasetWithProperties result = new DatasetWithProperties(datasets.get(0)); | ||
| Optional<String> patientID = openbis.findPropertyInSampleHierarchy("PATIENT_DKFZ_ID", | ||
| result.getExperiment().getIdentifier()); | ||
| patientID.ifPresent(s -> result.addProperty("patientID", s)); |
There was a problem hiding this comment.
If multiple properties will be set a distinct mapping could also make sense down the line (via an Enum e.g.)
| public class Time { | ||
| @JsonProperty | ||
| String unit; | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "Time{" + | ||
| "unit='" + unit + '\'' + | ||
| '}'; | ||
| } |
There was a problem hiding this comment.
Shouldn't this also have a value, or is this only the unit specification?
There was a problem hiding this comment.
one would think so, but the example yaml only has unit
| Path path = Paths.get(Objects.requireNonNull(findYaml(new File(outputPath))).getPath()); | ||
| Charset charset = StandardCharsets.UTF_8; | ||
|
|
||
| String idInLine = "openBISId:(.*)?(\\r\\n|[\\r\\n])"; |
There was a problem hiding this comment.
Since you reuse the openBISId: string it might make sense to provide it as a final constant (within this method is more than fine though)
| System.out.println(META_INFO_YAML + " not found"); | ||
| return null; |
There was a problem hiding this comment.
NIT: This currently does not catch the case on what happens if the user provides multiple "metaInformation.yaml" by mistake.
There was a problem hiding this comment.
good point, I think the most top level yaml with that name should be considered the correct one, as it is now
|
What is this for? Why is this needed? What does this change? |
No description provided.