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

Ignore mypy issue on jinja2.contextfunction function #1554

Merged
merged 4 commits into from Mar 27, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Mar 26, 2022

jinja2 released a new version, and as a consequence our pipeline broke.

EDIT: we need to pin jinja2 on our pipeline because mkdocs doesn't work with the latest jinja2.

@Kludex Kludex requested a review from a team March 26, 2022 19:13
@Kludex Kludex changed the title Ignore mypy issue on jinja2 function Pin jinja2 to be able to build with mkdocs Mar 26, 2022
@Kludex Kludex changed the title Pin jinja2 to be able to build with mkdocs Pin jinja2 to be able to build our docs Mar 26, 2022
requirements.txt Outdated Show resolved Hide resolved
@Kludex Kludex changed the title Pin jinja2 to be able to build our docs Ignore mypy issue on jinja2.contextfunction function Mar 26, 2022
@Kludex Kludex requested a review from adriangb March 27, 2022 18:06
pass_context = jinja2.contextfunction
pass_context = jinja2.contextfunction # type: ignore[attr-defined]
Copy link
Member

@adriangb adriangb Mar 27, 2022

Choose a reason for hiding this comment

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

Could you clarify why this is necessary? Is Jinja2 untyped / the types are wrong? I'm going to run this locally also, going to take me a sec tho

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The attribute is not defined on the latest jinja2 (which is the one installed for Python 3.7+).

The conditional there is because Python 3.6 will have a previous jinja2 version installed.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. It's just missing in newer versions of Jinja. And so MyPy complains that it's missing. Seems like a reasonable ignore.

Copy link
Member

Choose a reason for hiding this comment

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

Can you use sys.version_info instead?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I guess we could... I've added a note on the PR that removes Python 3.6 (#1357) to remove this logic anyway.

Copy link
Member

@adriangb adriangb Mar 27, 2022

Choose a reason for hiding this comment

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

I thought about that but it seems like it could lead to issues because the coupling between the Python version and Jinja2 version is coming from our requirements file, so it's pretty much an implementation detail. If there were a sys.version_info but for jinja that is what we would want to do

@Kludex Kludex merged commit 710aa62 into master Mar 27, 2022
@Kludex Kludex deleted the fix/jinja2-support-again branch March 27, 2022 18:17
@Kludex
Copy link
Sponsor Member Author

Kludex commented Mar 27, 2022

Thanks @adriangb :)

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.

None yet

3 participants