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.

Sign up for GitHub

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

Jump to bottom

Update template files to comply with linter jobs. #249

Merged
merged 3 commits into from
Nov 4, 2024
Merged

Update template files to comply with linter jobs. #249

merged 3 commits into from
Nov 4, 2024

Conversation

lrcouto
Copy link
Contributor

@lrcouto lrcouto commented Oct 31, 2024 •
edited
Loading

Motivation and Context

While working on moving all of the project dependencies to pyproject.toml files as part of kedro-org/kedro#4116, our linting CI tasks started pointing out some errors:

src/project_dummy/pipeline_registry.py:2:1: UP035 typing.Dict is deprecated, use dict instead
src/project_dummy/pipeline_registry.py:8:29: UP006 [*] Use dict instead of Dict for type annotation
src/project_dummy/pipelines/data_science/nodes.py:2:1: UP035 typing.Dict is deprecated, use dict instead
src/project_dummy/pipelines/data_science/nodes.py:2:1: UP035 typing.Tuple is deprecated, use tuple instead
src/project_dummy/pipelines/data_science/nodes.py:10:48: UP006 [*] Use dict instead of Dict for type annotation
src/project_dummy/pipelines/data_science/nodes.py:10:57: UP006 [*] Use tuple instead of Tuple for type annotation

PEP 585 has proposed using Python builtin type hints instead of Dict, Tuple, and Set from the typing library, to simplify and avoid duplication. This did not appear as an linter issue until the dependencies were moved, perhaps due to the way Ruff deals with package resolution.

The linting tasks also accused some errors on the formatting of Cookiecutter placeholder lines on pyproject.toml itself, mostly demanding that they be put in double quotes.

Separating this PR from #246 to keep the two issues separate and more organized. #246 would depend on this one to be closed.

How has this been tested?

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Assigned myself to the PR
  • Added tests to cover my changes

@lrcouto
Lint?
755273b 
Signed-off-by: Laura Couto <[email protected]>
@lrcouto
Revert "Lint again"
a3e26fa 
This reverts commit 088365d.

Signed-off-by: Laura Couto <[email protected]>
@lrcouto
Replace all Set, Tuple and Dict uses by python builtin type hints
e8dbc00 
Signed-off-by: Laura Couto <[email protected]>
@lrcouto lrcouto marked this pull request as ready for review October 31, 2024 16:35
@lrcouto lrcouto requested review from noklam, merelcht, DimedS, astrojuanlu and ElenaKhaustova and removed request for noklam and merelcht October 31, 2024 16:35
@lrcouto lrcouto changed the title Comply with linter Update template files to comply with linter jobs. Oct 31, 2024
@lrcouto lrcouto mentioned this pull request Oct 31, 2024
Merged
3 tasks
merelcht
merelcht reviewed Nov 1, 2024
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

Can you just double check that adding the double quotes to the cookiecutter placeholders doesn't change the behaviour?

astro-airflow-iris/{{ cookiecutter.repo_name }}/pyproject.toml
@@ -8,7 +8,7 @@ readme = "README.md"
dynamic = ["dependencies", "version"]

[project.scripts]
{{ cookiecutter.repo_name }} = "{{ cookiecutter.python_package }}.__main__:main"
"{{ cookiecutter.repo_name }}" = "{{ cookiecutter.python_package }}.__main__:main"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the quotes not change any of the behaviour? We don't have these in the base template either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question. Is this a correct fix, it looks like ruff is treating this as pyproject.toml but it is using jinja syntax that ruff will not understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not change the behavior from what I'm seeing here. I've tested it by creating a new project with a starter and using this branch as the --checkout argument. It resolves the repo name correctly and kedro run works as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it locally as well and it does seem to work fine. I specifically checked that the resulting pyproject.toml file looked the same as when using the released version.

ElenaKhaustova
ElenaKhaustova approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@lrcouto lrcouto merged commit 7a263e9 into main Nov 4, 2024
27 checks passed
@lrcouto lrcouto deleted the comply-with-linter branch November 4, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElenaKhaustova ElenaKhaustova ElenaKhaustova approved these changes

@merelcht merelcht merelcht approved these changes

@noklam noklam Awaiting requested review from noklam

@DimedS DimedS Awaiting requested review from DimedS

@astrojuanlu astrojuanlu Awaiting requested review from astrojuanlu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lrcouto @noklam @merelcht @ElenaKhaustova