Skip to content
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

Feature/normalise function - tweak the name of the function that standardises input number format #20

Merged
merged 10 commits into from
Jun 12, 2023

Conversation

andylaw
Copy link
Collaborator

@andylaw andylaw commented Jun 12, 2023

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

@pacharanero
Copy link
Contributor

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?

@andylaw
Copy link
Collaborator Author

andylaw commented Jun 12, 2023

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).

@pacharanero
Copy link
Contributor

pacharanero commented Jun 12, 2023 via email

@andylaw
Copy link
Collaborator Author

andylaw commented Jun 12, 2023

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

…ype annotation from 3.10 (details of this change in PEP604)
@andylaw
Copy link
Collaborator Author

andylaw commented Jun 12, 2023

Good save, sir!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants