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.

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

Add missing type hints to Jinja2Templates() #2157

Merged
merged 6 commits into from
May 31, 2023
Merged

Conversation

mattaw
Copy link
Contributor

@mattaw mattaw commented May 29, 2023

Summary

FileSystemLoader() from jinja2 (see its comment block) has supported both a single str / PathLike or a
sequence of them for at least four years. Starlette currently only has typehints for a single str / PathLike.

Mypy passes. Documented and test created.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

FileSystemLoader() from jinja2 supports a single or sequence of str or PathLike
@Kludex
Copy link
Member

Kludex commented May 30, 2023

Can you add a test that proves it, please?

@mattaw
Copy link
Contributor Author

mattaw commented May 30, 2023

Currently the patch changes no logic at all and appears thoroughly tested by existing starlette tests. Additionally, mypy verifies the types hints thoroughly from starlette's API through to Jinja2's. Jinja2 has type hints and tests for sequences already which mypy propagates through starlette.

On that basis, what extra tests are needed?

Thanks!

@Kludex
Copy link
Member

Kludex commented May 30, 2023

We are interested in making sure that directory on Jinja2Templates does accept what the type hints are suggesting. The underlying implication doesn't matter here. If Jinja2 makes a release that breaks when you use a sequence (even if the types are correct), or if we change the package we use on Jinja2Templates (well... unlikely... but it doesn't matter).

@mattaw
Copy link
Contributor Author

mattaw commented May 30, 2023

Thanks for the info, please find both a test and a very minor add to the docs. I attempted to copy the existing styles, variable names as closely as I could.

Happy to take any and all feedback.

If you are happy with it, and want me to squash it before committing please let me know.

@Kludex
Copy link
Member

Kludex commented May 30, 2023

No squashs are needed from your behalf.

Thanks. 🙏

@Kludex
Copy link
Member

Kludex commented May 31, 2023

It's a bit troublesome when you create PRs from the fork's main branch.

I can't push code to this branch. Can you apply these changes, please: a46a825 ?

@mattaw
Copy link
Contributor Author

mattaw commented May 31, 2023

I'm sorry, when I read the development docs I don't remember seeing any guidance.
As this is kind of a 'bug fix' I went ahead and created the PR on master, but I can happily rebase it onto something else?
(Also I have found, for a first contribution, it's easier to have a discussion around an actual PR as details matter and maintainers time is short. As someone involved in managing a larger project do you think that's a good thought or what a better approach would be?)

@Kludex
Copy link
Member

Kludex commented May 31, 2023

I just need the changes on that commit to be here.

(Also I have found, for a first contribution, it's easier to have a discussion around an actual PR as details matter and maintainers time is short. As someone involved in managing a larger project do you think that's a good thought or what a better approach would be?)

Hmm... It's fine the way you did, it's really just the branch, because it's protected, so I can't push there.

@mattaw
Copy link
Contributor Author

mattaw commented May 31, 2023

Nice refactor, it is much clearer than my patch. As Jinja can read files from subdir's of template directories I was being overly cautious in asserting that one directory was not the child or the same as the other.

I shortened the html templates so black does not split them into 3 lines due to their line length, and the template text does not have to say Hello :).

I also found that template_a.html and template_b.html were being created in the same directory due to a typo and fixed it.

@Kludex
Copy link
Member

Kludex commented May 31, 2023

Thanks @mattaw 🙏

@Kludex Kludex enabled auto-merge (squash) May 31, 2023 15:54
@Kludex Kludex merged commit 22575e6 into encode:master May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants