Update project to python 3.11 and new linting #175
Conversation
Why these changes are being introduced: We are systematically moving through our python applications and updating the python version, linting configurations, and some other general scaffolding. With this project likely touched for some upcoming projects, it was decided to update in advance of that work. How this addresses that need: * bumps python version to 3.11 * adds ruff and pyproject.toml approach to linting * code style and conventions updated to match new linting rules Side effects of this change: The python version was updated from 3.10 to 3.11. Tests have completed fine and testing will be performed in Dev1, but majoy python versions always come with some risk of unintended changes. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/IN-894
Pull Request Test Coverage Report for Build 6590799247
💛 - Coveralls |
| @@ -50,6 +49,8 @@ coverage.xml | |||
| *.py,cover | |||
| .hypothesis/ | |||
| .pytest_cache/ | |||
| cover/ | |||
There was a problem hiding this comment.
Is this correct or a coverage typo?
There was a problem hiding this comment.
Hmmm, @jonavellecuerdo, any insights here? I took this from template verbatim, making sure anything important from the old file was maintained.
There was a problem hiding this comment.
When creating the .dockerignore, I essentially copied contents of the existing .gitignore from the template repos. See note in PR for Python CLI template repo. So I think it should be fine!
| @@ -6,7 +6,11 @@ | |||
|
|
|||
|
|
|||
| def generate_extract_command( | |||
| input_data: dict, run_date: str, timdex_bucket: str, verbose: bool | |||
| # ruff: noqa: FBT001 | |||
There was a problem hiding this comment.
This is an interesting error code, I think we should consider respecting this with new apps but no need for refactoring here at the moment
There was a problem hiding this comment.
Which error code? The ruff linting rule FBT001?
Yeah, as we migrate projects, been seeing that. I can understand the logic behind not using positional boolean arguments in a function, but sometimes it does seem appropriate. As we ease into these new linting rules, I'm quite happy with this being an explicit skip for the time being when the situation calls for it, but not necessarily at the file or project level, because I do think it helps pause and think if a bool arg is a good idea.
| @@ -7,7 +7,7 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| def lambda_handler(event: dict, context: dict) -> dict: # noqa | |||
| def lambda_handler(event: dict, _context: dict) -> dict: | |||
There was a problem hiding this comment.
Was the underscore to address a linting error or preference? 🤔
There was a problem hiding this comment.
Yes: because context is not used, it passes linting that way.
|
@ghukill Thank you for updating this repo! Just one comment/question. |
What does this PR do?
Helpful background context
We are systematically moving through our python applications and updating the python version, linting configurations, and some other general scaffolding.
With this project likely touched for some upcoming projects, it was decided to update in advance of that work.
How can a reviewer manually see the effects of these changes?
Kickoff a new TIMDEX StepFunction run in Dev1 with the following input payload:
{ "next-step": "extract", "run-date": "2023-10-20", "run-type": "daily", "source": "libguides", "oai-pmh-host": "https://libguides.mit.edu/oai.php", "oai-metadata-format": "oai_dc", "oai-set-spec": "guides" }Note the pipeline lambdas, which are invoked multiple times in the pipeline, are returning correctly formatted commands for the next pipeline step.
Or, view this run which was started after these changes were deployed to Dev1.
Includes new or updated dependencies?
YES
What are the relevant tickets?
https://mitlibraries.atlassian.net/browse/IN-894
Developer
Code Reviewer
(not just this pull request message)