Closed andylaw closed 1 year ago
Looks good code-wise, and I like the warnings.warn
in normalise_number()
However, on Python 3.7 this PR causes errors on running the tests because of the line:
def standardise_format(nhs_number: str | int) -> str:
<-- I think this syntax is from 3.10
I kept to a fairly old Python version for the time being because wasn't sure how recent a Python version we are going to move to. If we are moving to only supporting >3.10 then we can leave this as-is. If we want to maybe include some older versions that might plausibly be in use we could aim for >3.8?
Thoughts?
I can't install Python 3.7 on my MacBook because it's an M2 chip and there isn't a version available for it that I've found via conda at least. I'll maybe look at Docker options. We should probably look to maintain 3.7 as a minimum, even though it is end-of-life soon (as you pointed out elsewhere, I think).
OK how about I try to fix it here on 3.7 and I'll update the PR accordingly. The rest of the code review was fine and I can then merge.
I will look into ways we can make GitHub Actions run the tests against a matrix of our supported Python versions, as a pre-approval check which must pass before deployment to test.pypi.org or pypi.org(live) can occur. I figure this is the setup you had with Travis, as there is a list of versions in the Travis CI config file.
On Mon, 12 Jun 2023 at 13:37, Andy Law @.***> wrote:
I can't install Python 3.7 on my MacBook because it's an M2 chip and there isn't a version available for it that I've found via conda at least. I'll maybe look at Docker options. We should probably look to maintain 3.7 as a minimum, even though it is end-of-life soon (as you pointed out elsewhere, I think).
— Reply to this email directly, view it on GitHub https://github.com/uk-fci/nhs-number/pull/20#issuecomment-1587259556, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR5KYJRKL7ADCJHBKPRS6TXK4EQVANCNFSM6AAAAAAZDH4EGA . You are receiving this because your review was requested.Message ID: @.***>
OK how about I try to fix it here on 3.7 and I'll update the PR accordingly. The rest of the code review was fine and I can then merge.
Perfect. Thanks
Good save, sir!
Code changes to rename the function (and the enclosing file), allow pre-existing code to continue to work but be issued a deprecation warning, handle int arguments for a particular edge case