ENH/BUG/MAINT: support comments/blank lines and better validation in "per-sample sequence" formats #136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…"per-sample sequence" formats All changes in this commit are related to the "per-sample sequence" formats. Several changes: - Comments and blank lines are now supported in "per-sample sequence" manifest formats, including sniffers and transformers. - Duplicate samples are now additionally detected and disallowed in "relative path" manifest formats (previously this validation only happened in the "absolute path" manifest formats). - Error messages reported by transformers are both more precise and accurate. - Refactored sniffer and transformer code to reduce redundancy and perform consistent validation and parsing in all sniffers/transformers. - Added unit tests, increased coverage to 100% for "per-sample sequence" formats, and made existing unit tests and test data more robust. Fixes qiime2#126. Fixes qiime2#129. Fixes qiime2#132. Pair-programmed with @jairideout.
thermokarst
reviewed
Jul 24, 2017
| @@ -0,0 +1,4 @@ | |||
| sample-id,absolute-filepath,direction | |||
| Human-Kneecap,/some/groovy/folder/Human-Kneecap_S1_L001_R1_001.fastq.gz,forward | |||
thermokarst
approved these changes
Jul 24, 2017
There was a problem hiding this comment.
I have no comments guys, besides the fact that this PR looks great! Thanks so much for jamming on this 🎸! This is nice and clean, and easy to follow --- looking forward to all this centralized/standardized validation (does that make this the validation station 🚋 ?)!
| os.path.expandvars(record['absolute-filepath']) | ||
| _validate_path(record['absolute-filepath']) | ||
| _validate_direction(record['direction']) | ||
| raise ValueError('Empty cells are not supported in ' |
| path = record[expected_header[1]] | ||
| if absolute: | ||
| if not os.path.isabs(path): | ||
| raise ValueError('All paths provided in manifest must be ' |
| 'absolute but found relative path: %s' % path) | ||
| else: | ||
| if os.path.isabs(path): | ||
| raise ValueError('All paths provided in manifest must be ' |
| path = os.path.join(os.path.dirname(manifest_fh.name), path) | ||
| if not os.path.exists(path): | ||
| raise FileNotFoundError( | ||
| 'A path specified in the manifest does not exist ' |
| def _validate_header(manifest, expected_header): | ||
| header = manifest.columns.tolist() | ||
| if header != expected_header: | ||
| raise ValueError('Expected manifest header %r but ' |
| 'but the following directions were observed: %s' | ||
| % ', '.join(directions)) | ||
|
|
||
| 'contain only forward or reverse reads, ' |
| @@ -252,7 +236,7 @@ def _validate_paired_end_fastq_manifest_directions(manifest): | |||
| elif direction == 'reverse': | |||
| reverse_direction_sample_ids.append(sample_id) | |||
| else: | |||
| raise ValueError('Directions can only be "forward" and ' | |||
| raise ValueError('Directions can only be "forward" or ' | |||
| for file in files: | ||
| filepath = self.get_data_path(file) | ||
| for format in self.formats: | ||
| with self.assertRaisesRegex(ValueError, format.__name__): |
| 'relative_manifests/jagged-MANIFEST'] | ||
| for file in files: | ||
| filepath = self.get_data_path(file) | ||
| with self.assertRaisesRegex(ValueError, 'FastqManifestFormat'): |
| @@ -13,15 +13,74 @@ | |||
| from ..plugin_setup import plugin | |||
|
|
|||
|
|
|||
| class FastqManifestFormat(model.TextFileFormat): | |||
| class _FastqManifestBase(model.TextFileFormat): | |||
There was a problem hiding this comment.
Sweet, I like this game plan.
ebolyen
pushed a commit
that referenced
this pull request
Jul 24, 2017
Missing test assets from #136
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All changes in this commit are related to the "per-sample sequence" formats.
Several changes:
Fixes #126.
Fixes #129.
Fixes #132.
Pair-programmed with @jairideout .