-
Notifications
You must be signed in to change notification settings - Fork 17
add missing eol at eof so github does not complain #172
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
Conversation
...te/.github/workflows/{% if mypy_type_checking != 'none' %}type-checking.yml{% endif %}.jinja
Show resolved
Hide resolved
| with: | ||
| src: ./src | ||
| {% endif %} | ||
| {% endif -%} |
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.
Same a prior comment: Wouldn't this remove the new line at the end of the file when the template is hydrated?
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 removes the following newline (after the %} in the same line) which leaves the newline at the end of ./src
which is what we want
| {%- if create_example_module -%} | ||
| from .example_module import * | ||
| {% endif %} | ||
| {% endif -%} |
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.
Just to confirm, you checked this either manually or with CI for the following four cases of hydrated templates:
- pylint with example module
- pylint without example module
- black with example module
- black without example module
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.
All of these passed the CI checks when I committed. Is that sufficient?
I do not trust my local testing as it is easy to make mis steps.
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.
A new PR #174 has been merged that expands the CI to include more cases (we avoided doing this in the past because of how quickly this can explode the number of tests) but for here and now, we might as well make use of it.
I'm not sure how to get this PR to run all 12 test cases. Hopefully there's a nicer way that the brute force approach of closing and recreating the PR.
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 made a gratuitous whitespace change and forced the rebuild. I think we might add this line to ci.yml:
on: workflow_dispatch
Reportedly enables manual running actions
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_dispatch
drewoldag
left a comment
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.
Looks good to me. Thanks for putting this together.
I noticed these when creating a new project using the template.