Handle 4 best practice proforma sequences#33
Conversation
…or database keys by relaxing tag requirements
|
I like how you're relaxing the parsing, waiting until later for stricter validation. The space handling looks good (ditching spaces before the key, keeping all the rest). I might recommend storing the whitespace following the value in a separate property, so that it can be reproduced, but so that the user doesn't need to stick I'll take a look at those best practice tests tomorrow afternoon. |
| Assert.AreEqual(ProFormaKey.Mod, tag25.Descriptors[0].Key); | ||
| Assert.AreEqual("p-adenosine", tag25.Descriptors[0].Value); | ||
| Assert.AreEqual(ProFormaKey.Mod, tag25.Descriptors[1].Key); | ||
| Assert.AreEqual(" N6-(phospho-5'-adenosine)-L-lysine(RESID)", tag25.Descriptors[1].Value); |
There was a problem hiding this comment.
Should this have the space trimmed off of the left?
There was a problem hiding this comment.
I think this should be trimmed, so that using string comparisons with modifications in a database would give expected results.
There was a problem hiding this comment.
Open question in my opinion. Right now I'm only trimming from the ends of the descriptor. I am preserving all of the spaces directly after the colon. Seems to be the middle ground and I'm not sure if that is good. We could aggressively trim (which speaks to me), but one could argue that would hurt readability when written back out. We could not trim at all, but that complicates matching key and value strings in the backend.
I'd vote we accept this PR and discuss during the next meeting ... I've got a couple more PRs lined up after this one :)
There was a problem hiding this comment.
Didn't see your response a minute later ... I don't mind switching to "aggressive trimming" for this PR. But a group discussion is warranted.
2 participants
Adds some unit tests and relaxes the requirements for tags (from enum to string). I'm wanting feedback here: my plan is to relax the key requirements in this step and handle the higher level logic during validation and conversion. For example, a tag of
[fake:123]would pass this stage and then throw an exception during a conversion to a proteoform. There was also the practical issue of PSI-MOD not working as an enum value. There is also a unit test calledHandleExtraTagSpaces()that explicitly tests for extra space handling; I'm curious to see what people think. This PR addresses #22 and #27