Conversation
oylenshpeegul
left a comment
oylenshpeegul
left a comment
There was a problem hiding this comment.
This looks great! The new CTV3 stuff is very much parallel with the other codelist types. The new documentation is fantastic.
I only spotted one tiny change in a test.
rust/codelist-rs/src/types.rs
Outdated
| assert!(matches!(CodeListType::from_str("ICD10"), Ok(CodeListType::ICD10))); | ||
| assert!(matches!(CodeListType::from_str("SNOMED"), Ok(CodeListType::SNOMED))); | ||
| assert!(matches!(CodeListType::from_str("OPCS"), Ok(CodeListType::OPCS))); | ||
| assert!(matches!(CodeListType::from_str("ctv3"), Ok(CodeListType::CTV3))); |
There was a problem hiding this comment.
This one was meant to be "CTV3" (upper case), right?
There was a problem hiding this comment.
Yes great spot! Will change this now.
CarolineMorton
left a comment
CarolineMorton
left a comment
There was a problem hiding this comment.
Looks good! Nice work on this.
Approving but added one small comment on the Metadata::default()
| //! CTV3 validator for validating CTV3 codes in a codelist | ||
| //! | ||
| //! Validation Rules | ||
| //! 1. The code must be exactly 5 characters in length. | ||
| //! 2. Only alphanumeric characters (a-z, A-Z, 0-9) and dots (.) are allowed. | ||
| //! 3. The code starts with 0-5 alphanumeric characters followed by dots to pad to 5 characters. |
There was a problem hiding this comment.
Excellent documentation. Thanks for the effort here. It is always nice to have this right off the bat.
| fn create_test_metadata() -> Metadata { | ||
| Metadata::new( | ||
| Provenance::new(Source::ManuallyCreated, None), | ||
| CategorisationAndUsage::new(None, None, None), | ||
| PurposeAndContext::new(None, None, None), | ||
| ValidationAndReview::new(None, None, None, None, None), |
There was a problem hiding this comment.
I think we can simplify this with the changes from #68 that @oylenshpeegul added. We should be able to do
Metadata::default(),There was a problem hiding this comment.
Ah yes will change this now, thanks!
| fn test_validate_invalid_code_dot_middle_character_between_letters() -> Result<(), CodeListError> | ||
| { | ||
| let codelist = create_test_codelist()?; | ||
| let validator = Ctv3Validator(&codelist); | ||
| let code = "10a.f"; |
There was a problem hiding this comment.
Great set of tests.
| //! OPCS validator for validating OPCS codes in a codelist | ||
| //! | ||
| //! Validation Rules | ||
| //! 1. The code must be 3-5 characters long. | ||
| //! 2. The first character must be a letter. | ||
| //! 3. The second and third characters must be numbers. | ||
| //! 4. If there is a fourth character and it is a dot, there must be a number after the dot. |
There was a problem hiding this comment.
Really appreciate this.
3 participants
closes #90
From what I could gather through a bit of research, the rules for CTV3 are:
E.g. valid codes:
ABCDE
abCdE
A....
ab9..
E.g. invalid codes:
ABC
ABCDEF
..jk0
KL..0
Have also added validation rules to docs (for all but CTV3 I copied the rules stated in the tre-tools code)
Also added a missing test to ICD10 validator to check letters are uppercase