-
Notifications
You must be signed in to change notification settings - Fork 61
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
Update template files to comply with linter jobs. #249
Conversation
Signed-off-by: Laura Couto <[email protected]>
This reverts commit 088365d. Signed-off-by: Laura Couto <[email protected]>
Signed-off-by: Laura Couto <[email protected]>
There was a problem hiding this 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?
@@ -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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
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:PEP 585 has proposed using Python builtin type hints instead of
Dict
,Tuple
, andSet
from thetyping
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