From ae6c53aa69e4a98e4be17aadc5aaee173a360ace Mon Sep 17 00:00:00 2001 From: Jim Rybarski Date: Sat, 19 Nov 2022 17:54:48 -0600 Subject: [PATCH 1/2] Retain spaces when splitting long lines When long qualifier values are split on spaces to keep the total line length less than 80 characters, and the final character on a line is a space, that space is deleted. However, this prevents parsing the original value - it's not possible to know when joining the lines together whether a space should be inserted or not. For example: Lipopolysaccharide export system ATP-binding protein LptB will appear in the Genbank text as: /product="Lipopolysaccharide export system ATP-binding protein LptB" and when this is read later and newlines are removed, it will be: Lipopolysaccharide export system ATP-bindingprotein LptB This commit retains the final space at the end of the line so that the original value can be reconstructed unambiguously. --- src/writer.rs | 46 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/src/writer.rs b/src/writer.rs index a8cfa32..bf4e9fc 100644 --- a/src/writer.rs +++ b/src/writer.rs @@ -300,11 +300,10 @@ fn wrap_location( } /// Wrap a line of text in a Genbank file. The line is appended to `line`, which -/// may already contain up to `max_len` - 1 bytes. Wrapping occurs on a space if -/// possible. Outputs valid UTF-8, however lines are wrapped to their length in -/// bytes, not characters, and UTF-8 characters that consist of multiple `char`s -/// are likely to get mangled. Returns the rest of `input`. If `quote` is set, -/// `"` will be escaped with `""` +/// may already contain up to `max_len` - 1 bytes. Outputs valid UTF-8, however +/// lines are wrapped to their length in bytes, not characters, and UTF-8 +/// characters that consist of multiple `char`s are likely to get mangled. +/// Returns the rest of `input`. If `quote` is set, `"` will be escaped with `""` fn wrap_get_line<'a>(line: &mut String, input: &'a str, max_len: usize, quote: bool) -> &'a str { assert!(line.len() < max_len); let mut consumed = 0; @@ -356,7 +355,7 @@ fn wrap_get_line<'a>(line: &mut String, input: &'a str, max_len: usize, quote: b // try to wrap at last space if let Some(last_space_in) = last_space_in { line.truncate(last_space_out); - &input[last_space_in + 1..] + &input[last_space_in..] } else { // there's no way to split the line nicely &input[consumed..] @@ -400,7 +399,40 @@ fn wrap_text( pub mod tests { use super::*; - use std::io::BufRead; + use crate::reader::SeqReader; + use crate::seq::{Feature, Location, Seq}; + use std::io::Read; + use std::io::{BufRead, BufReader}; + + #[test] + fn multiline_spaces_retained_after_roundtrip() { + // ensures that qualifier values that are split across multiple lines when writing + // don't lose any spaces when they're parsed again + let product = "Lipopolysaccharide export system ATP-binding protein LptB".to_string(); + let feat = Feature { + kind: feature_kind!("CDS"), + location: Location::simple_range(100, 200), + qualifiers: vec![(qualifier_key!("product"), Some(product.clone()))], + }; + let mut seq = Seq::empty(); + seq.features = vec![feat]; + let mut out = Vec::new(); + SeqWriter::new(&mut out).write(&seq).unwrap(); + let mut text = String::new(); + std::io::Cursor::new(&out) + .read_to_string(&mut text) + .unwrap(); + + let buf = BufReader::new(text.as_bytes()); + let mut roundtrip_seq = SeqReader::new(buf); + let record = roundtrip_seq.next().unwrap().unwrap(); + let roundtrip_product = record.features[0] + .qualifier_values(qualifier_key!("product")) + .next() + .unwrap() + .replace('\n', ""); + assert_eq!(product, roundtrip_product); + } #[test] fn truncate_locus() { From 272b9e9cd920d6ee7a5c85829d7c904cd46db993 Mon Sep 17 00:00:00 2001 From: Jim Rybarski Date: Thu, 24 Nov 2022 10:19:00 -0600 Subject: [PATCH 2/2] Fix all clippy warnings This commit implements all the changes recommended by `cargo clippy`, which is currently causing the CI pipeline to fail. --- src/reader/streaming_parser.rs | 20 ++++++++++---------- src/seq.rs | 16 ++++++++-------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/reader/streaming_parser.rs b/src/reader/streaming_parser.rs index cf571bd..f5af646 100644 --- a/src/reader/streaming_parser.rs +++ b/src/reader/streaming_parser.rs @@ -238,8 +238,8 @@ impl StreamParser { pub fn read_one_record(&mut self) -> Result, GbParserError> { // skip preamble such as the header of Genbank .SEQ files - self.try_run_parser(&skip_preamble, false)?; - let locus = match self.run_parser(&locus, true) { + self.try_run_parser(skip_preamble, false)?; + let locus = match self.run_parser(locus, true) { Ok(locus) => locus, Err(StreamParserError::EOF) => { return Ok(None); @@ -257,14 +257,14 @@ impl StreamParser { division: locus.division, ..Seq::empty() }; - let fields = self.run_parser_many0(&any_field)?; + let fields = self.run_parser_many0(any_field)?; let mut seq = fill_seq_fields(seq, fields).map_err(GbParserError::SyntaxError)?; //TODO: Proper error handling - if self.try_run_parser(&features_header, true)?.is_some() { - seq.features = self.run_parser_many0(&feature)?; + if self.try_run_parser(features_header, true)?.is_some() { + seq.features = self.run_parser_many0(feature)?; } - self.try_run_parser(&base_count, true)?; - seq.contig = self.try_run_parser(&contig_text, true)?; - if self.try_run_parser(&origin_tag, true)?.is_some() { + self.try_run_parser(base_count, true)?; + seq.contig = self.try_run_parser(contig_text, true)?; + if self.try_run_parser(origin_tag, true)?.is_some() { seq.seq = self.parse_seq_data(seq.len)?; } @@ -274,8 +274,8 @@ impl StreamParser { return Ok(Some(seq)); } - self.run_parser(&double_slash, true)?; - self.run_parser_many0(&line_ending_type_hack)?; + self.run_parser(double_slash, true)?; + self.run_parser_many0(line_ending_type_hack)?; Ok(Some(seq)) } } diff --git a/src/seq.rs b/src/seq.rs index 0bae6db..8cd2c37 100644 --- a/src/seq.rs +++ b/src/seq.rs @@ -12,7 +12,7 @@ use crate::dna::revcomp; pub use crate::{FeatureKind, QualifierKey}; #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] /// A very simple Date struct so we don't have to rely on chrono pub struct Date { year: i32, @@ -20,7 +20,7 @@ pub struct Date { day: u32, } -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] pub struct DateError; impl Date { @@ -66,7 +66,7 @@ impl fmt::Display for Date { } #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, PartialEq, Clone, Copy)] +#[derive(Debug, PartialEq, Eq, Clone, Copy)] pub enum GapLength { /// gap(n) Known(i64), @@ -77,10 +77,10 @@ pub enum GapLength { } #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, PartialEq, Clone, Copy)] +#[derive(Debug, PartialEq, Eq, Clone, Copy)] pub struct Before(pub bool); #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, PartialEq, Clone, Copy)] +#[derive(Debug, PartialEq, Eq, Clone, Copy)] pub struct After(pub bool); /// Represents a Genbank "location", used to specify the location of @@ -351,7 +351,7 @@ impl Feature { } #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] pub enum Topology { Linear, Circular, @@ -368,14 +368,14 @@ impl fmt::Display for Topology { } #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] pub struct Source { pub source: String, pub organism: Option, } #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone)] pub struct Reference { pub description: String, pub authors: Option,