Fix YAML validation + prepare for patch release #124
Conversation
|
Error seems to be related to the validation of YAML files that have no header. I'll try and fix this first |
awgymer
left a comment
awgymer
left a comment
There was a problem hiding this comment.
I think that part of the issue was that no tests really checked we performed the validation. Should we add some new tests to ensure this doesn't creep in again somehow?
(I think it might just be a case of having a test that we actually intend to fail validation - that can only happen if content is actually passed)
| fileContent = new Yaml().load((samplesheetFile.text)).collect { | ||
| if(containsHeader) { | ||
| return it as Map | ||
| } | ||
| return ["empty": it] as Map | ||
| } |
There was a problem hiding this comment.
Is the identical code in SamplesheetConverter:castToType still required?
There was a problem hiding this comment.
This isn't in that function? The main issue here is that we need an array of Map values for the validation. This is a little hacky workaround for samplesheets that have no header
There was a problem hiding this comment.
Line 73 of SamplesheetConverter.groovy no?
There was a problem hiding this comment.
I figure if we are pre-converting here we didn't need it there? But maybe I'm not following the flow properly
There was a problem hiding this comment.
Do you mean the code in SamplesheetConverter.convertToList()?.
This code here is for the validation, convertToList is creating the Nextflow channel, and we don't pass fileContent or fileContentCasted to this function, they are independent. If we want to reduce code, I would rather remove the validation from fromSamplesheet(), but this is opening the discussion about where to validate a sample sheet again and out of topic so don't mind me :D
There was a problem hiding this comment.
Yes sorry I mixed up castToType with convertToList!
Code deduplication was the goal, but agree it is out of scope for this PR/patch release if it's not just a case of deleting what's there
There was a problem hiding this comment.
Yeah code deduplication should really happen at one point but not here :p
Good point! I'll add a failing test :) |
plugins/nf-validation/src/test/nextflow/validation/PluginExtensionMethodsTest.groovy
Outdated
Show resolved
Hide resolved
3 participants
This PR contains a fix for the YAML validation skip reported in #121 and the preparation for a patch release