Conversation
james-bruten-mo
left a comment
There was a problem hiding this comment.
Thanks Yash, looks good.
The only thing I'm concerned about is removing the Optional and Union typing imports as we've recently hit issues with python versions not being up to date enough.
| import re | ||
| import subprocess | ||
| from datetime import datetime | ||
| from typing import Optional, Union |
There was a problem hiding this comment.
Can we leave this as using the old Optional and Union statements for the time being. We've already seen python compatibility issues for this in this file
There was a problem hiding this comment.
Ah, I see that os46-3 stack is still using Python 3.7.
To make future tidying easier, would it work if we add the annotation import from future, rather than keeping the legacy imports?
from __future__ import annotationsIts probably only useful to fix type annotations, not runtime logic.
| import yaml | ||
| from collections import defaultdict | ||
| from pathlib import Path | ||
| from typing import Dict, List, Optional, Set, Union |
There was a problem hiding this comment.
Same as above (although I think Dict and List changes are an earlier python, so maybe they can stay)
There was a problem hiding this comment.
list, dict, set, tuple are supported natively since Python 3.9 deprecating the capitalise aliases from typing module.
| from contextlib import contextmanager | ||
| from pathlib import Path | ||
| from tempfile import mkdtemp | ||
| from typing import Dict, List, Set, Tuple |
PR Summary
Sci/Tech Reviewer:
Code Reviewer:
Modernise and refactor the repository.
Code Quality Checklist
Testing
Security Considerations
AI Assistance and Attribution
Code Review