Skip to content

Minor refactor + typing info - #155

Merged
steppi merged 1 commit into
jupyterlite:mainfrom
Carreau:mypy
Apr 17, 2024
Merged

Minor refactor + typing info#155
steppi merged 1 commit into
jupyterlite:mainfrom
Carreau:mypy

Conversation

@Carreau

@Carreau Carreau commented Mar 22, 2024

Copy link
Copy Markdown
Collaborator

This adds mypy configuration to ignore voici (untyped), and slightly refactor to pass some kwargs explicitely.

It also pleases mypy.

@Carreau Carreau added the enhancement New feature or request label Mar 26, 2024
@Carreau
Carreau requested a review from steppi March 26, 2024 13:53

@agriyakhetarpal agriyakhetarpal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, @Carreau! I'm just helping @steppi a bit with code review on some of the pending PRs and I had a look at the changes here, and they look good on my end. Question: does adding from __future__ import annotations to the top of the file make sense too (and maybe with an accompanied isort rule as well after #156), considering it will be mandated with future Python versions someday? I don't think we're using anything complicated here that will break later.

@Carreau

Carreau commented Apr 3, 2024

Copy link
Copy Markdown
Collaborator Author

Thanks for the review.

I tend to avoid unnecessary from __future__ import annotations, I found that unless necessary they tend to hide bugs in older versions of Python, in particular at runtime if you use get_type_hints.

More particularly, from __future__ import annotations make annotation lazy and allow you to write things that are invalid – typically you can start using list[...] in types, but this is invalid on earlier python.

As for isort, no objections, but I thought the precommit would take care of it ? If not we can just add it.

@agriyakhetarpal

Copy link
Copy Markdown
Member

Thanks for the info, TIL that future-annotations can get us invalid types on earlier Python versions like that. I guess we would have to enable the isort rule before pre-commit starts to apply it across the codebase, but I suppose that's something to do in a separate PR.

This adds mypy configuration to ignore voici (untyped),
and slightly refactor to pass some kwargs explicitely.

It also pleases mypy.

@steppi steppi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. Sorry for forgetting about this.

@steppi
steppi merged commit 22771e6 into jupyterlite:main Apr 17, 2024
@Carreau

Carreau commented Apr 18, 2024

Copy link
Copy Markdown
Collaborator Author

No problem, thanks for the review and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants