Skip to content

Conversation

@wow-such-code
Copy link
Member

@wow-such-code wow-such-code commented Feb 21, 2022

  • Increase plugin versions to status of data-model-lib
  • Fix testing environment
  • Include support for alternative nanopore data structure and add tests
  • Allow properties with empty value in key-value pair in log files for easier troubleshooting. Empty properties should be handled when processing the parsed metadata and deciding which are mandatory.

@wow-such-code wow-such-code marked this pull request as ready for review May 4, 2022 15:13
@wow-such-code wow-such-code requested a review from a team as a code owner May 4, 2022 15:13
Copy link
Contributor

@Steffengreiner Steffengreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya @wow-such-code Nicely done! I have some questions and will approve once these are answered 👍

Comment on lines 163 to 171
Schema jsonSchema = loadSchemaFromStream(OxfordNanoporeInstrumentOutput.getSchemaAsStream())
// Step 2: validate against schema return if valid, throw exception if invalid
jsonSchema.validate(jsonObject)
} catch (ValidationException validationException) {
// Step 2.5: validate against second schema type
Schema jsonSchema2 = loadSchemaFromStream(OxfordNanoporeInstrumentOutputV2.getSchemaAsStream())
jsonSchema2.validate(jsonObject)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be made more explicit by making the multiple schema validations into a seperate method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pushed a suggestion

private static void checkPresenceOfFlowCellPosition(Map metadata, String flowCellEntry) {
if (!metadata.containsKey(flowCellEntry)) {
throw new MissingArgumentException("Could not find metadata information about the flow cell position.")
throw new RuntimeException("Could not find metadata information about the flow cell position.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason for exchanging the exception type here
I'd expect a missing argument to throw a MissingArgumentException?

Comment on lines +115 to +116
protected boolean handleCommonParameters(final ToolMetadata toolMetadata,
final AbstractCommand command) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about the spacing adaptions in this class? Is this formatted as intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have been something to do with the eclipse codestyle, if it was changed by me. I don't remember unfortunately

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this formatting is correct as it is a continuation of the previous line

Copy link
Contributor

@Steffengreiner Steffengreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done 👍

@wow-such-code wow-such-code merged commit 6e04854 into development May 5, 2022
wow-such-code added a commit that referenced this pull request May 6, 2022
* Sync with hotfix 1.9.2 (#69)

* Hotfix 1.9.1 (#66)

* Fix RST syntax error

* Remove orphaned print statement

* Update CL

* Bump version 1.9.1 -> 1.9.2

Co-authored-by: Steffengreiner <Steffen.Greiner@gmx.de>

* Update maxquant file parser (#74)

The summary file is now searched within the summary folder

* Remove qube files and Bump version to 1.11.0-SNAPSHOT

* Bump conf.py to current version

* Bump version to 1.11.0-SNAPSHOT

* Include groovydoc plugin in report generation (#59)

* Feature/update nanopore (#91)

* increase some versions, fix testing env

* add tests for alternative nanopore structure

* Add worflow triggers (#93)

* Add triggers

* Reorder git commands

* Use master as base, not main

* parser

* update workflows (#94)

* Set version to 1.10.6

* update pom

* Fix project setup (#96)

* add possibility of some metadata values being empty

* comments

* extract method to validate a single schema

Co-authored-by: Sven F <sven.fillinger@qbic.uni-tuebingen.de>
Co-authored-by: JohnnyQ5 <support@qbic.zendesk.com>
Co-authored-by: Steffengreiner <Steffen.Greiner@gmx.de>
Co-authored-by: Sven F <9976560+sven1103@users.noreply.github.com>

* Feature/update nanopore (#97)

* increase some versions, fix testing env

* add tests for alternative nanopore structure

* Add worflow triggers (#93)

* Add triggers

* Reorder git commands

* Use master as base, not main

* parser

* update workflows (#94)

* Set version to 1.10.6

* update pom

* Fix project setup (#96)

* add possibility of some metadata values being empty

* comments

* extract method to validate a single schema

* use data-model-lib release

Co-authored-by: Sven F <sven.fillinger@qbic.uni-tuebingen.de>
Co-authored-by: JohnnyQ5 <support@qbic.zendesk.com>
Co-authored-by: Steffengreiner <Steffen.Greiner@gmx.de>
Co-authored-by: Sven F <9976560+sven1103@users.noreply.github.com>

* Update CHANGELOG.rst

Co-authored-by: Tobias Koch <KochTobi@users.noreply.github.com>

Co-authored-by: Sven F <sven.fillinger@qbic.uni-tuebingen.de>
Co-authored-by: Steffengreiner <Steffen.Greiner@gmx.de>
Co-authored-by: jnnfr <jennifer.boedker@qbic.uni-tuebingen.de>
Co-authored-by: JohnnyQ5 <support@qbic.zendesk.com>
Co-authored-by: Sven F <9976560+sven1103@users.noreply.github.com>
Co-authored-by: Tobias Koch <KochTobi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants