-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CHI number validation #25
Comments
Thanks @mattstibbs we would definitely like to add this additional validation, we are open to a PR 😄 |
Question: is this the right behaviour? i.e. Alternatively, should CHI validation be an additional validation applied somehow, leaving |
I think perhaps if/when we get around to adding strict CHI number validation, this could be something like >>> import nhs_number
>>> nhs_number.is_valid("1406230002",
for_region=nhs-numberREGION_SCOTLAND,
sex="Male"
strict_chi=True)
True so that the
We would just document clearly that this is what you would need to do in order to check a CHI number properly. For additional syntactic sugar we could create a convenience function This os just one of many ways to implement, and I am happy for the implementer to add their own flavour. |
It would be very easy to overthink this around the phrase "plausible" date of birth For example - 2904240012. Not valid if the date of birth is 1924, but valid if it is 2024, but that's in the future so...?? Lots of glorious rabbit-holes to disappear down :) |
@pacharanero Just so I'm clear, would you suggest that we keep the default behaviour of And then add the option to apply strict CHI validation to |
Yes, that's what I'm suggesting, however I'm open to views, especially from anyone actively validating CHI numbers. I am given to understand that Python prefers 'explicit' over 'implicit' so having explicit flags for strict CHI checking seems more Pythonic? They don't like 'magic' do they, Python people 😉. Rubyists like me, we love magic. I have some colleagues in GP in Scotland and I will ask them to have a look and give their views. In general I'm keener to see working code than a lot of worrying about how to implement. It'll be fine as long as it works and is documented. |
Hate it. Mind you, I used to be (still am, under the hood) a Perl person ("There's more than one way to do it") |
You're probably more of a Rubyist than you think then! Ruby gives you all the ways to do it, and all the footguns - you as the user need to know where to point the gun :-) |
https://bmcbioinformatics.biomedcentral.com/articles/10.1186/1471-2105-10-221 |
I think the key is probably agreeing on what we mean by a 'valid' number. i.e. is a valid number just conformant with the general format and checksum, or is it only valid if it could feasibly be issued etc. There is probably an alternative argument to say that i.e. is this simple a format validation library, or is it a helper library for the way things are in reality |
Practically, you know whether it is a CHI number or H&C number based upon the range, so you shouldn't need additional arguments. 010 100 0000 to 311 299 9999 (used for CHI numbers in Scotland) Logic as follows: if cast to int of left 6 digits < 320,000 then check that if datetime.strptime throws an error. |
Hey guys. Been looking into your package as a replacement for our own in house solution, and I was curious if this conversation was resolved. If you don't mind me making a suggestion, I think I would take a binary approach and the number is either valid or it is not. If the date section isn't possible it's not valid and as such I would add the check to the is_valid function using a check for region, not give CHI numbers their own is valid wrapper. We are currently taking the approach mentioned by @VJ911 where we check for an error from strptime which I believe takes care of any leap year oddness. I would be of the opinion that to be a valid CHI number it must be in the range of CHI numbers and the first 6 digits must make up a valid date. Something along the lines of if for_region == 'RANGE_SCOTLAND':
try:
# Should start with ddmmyy date
datetime.strptime(nhs_number[:6], "%d%m%y")
except ValueError:
return False You could have is_valid() return a NhsNumber object (details.py) that had some extra attributes to cover the information supplied with CHI numbers. Valid is a current attribute, but you could include an in_range attribute so that CHI number in range but with impossible dates would be Could also return the sex rather than asking a user to specify it when calling is_valid so the user can run their own checks against that. On top of that, you could include a faliure_message to provide some feed back about the impossible date being the issue and any other issues that you might encounter during validation. {valid: False, in_range: True, sex: 'male', faliure_message: 'In CHI number range but date section impossible' } The major draw back would be that this would all constitute breaking changes if applied to is_valid() so it might need a detailed_is_valid() function or something 🤷♂️ |
Thanks @andyatterton for this, very valid points made and I think we should finish off the CHI implementation so it works for real-life use-cases such as yours. I don't think we have a huge user base of the library at this stage, so a breaking change that makes If anyone is up for making the update and creating a PR, I think we would be amenable to accepting that. If not, I will get around to this when I get around to it. |
CHI numbers conform to the standard NHS validation format, but have an additional requirement that the first 6 digits must be a valid date of birth (DDMMYY).
The validate functions should identify that a provided number falls into the Scotland range, and then apply the additional validation checks.
https://en.wikipedia.org/wiki/National_Health_Service_Central_Register#Community_Health_Index
This new validation should then be applied to the generate() function when a user passes in
for_region=nhs_number.REGION_SCOTLAND
so that only a valid CHI is generated.The text was updated successfully, but these errors were encountered: