Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add type annotations #23

Merged
merged 1 commit into from
Jul 21, 2024
Merged

Conversation

nstarman
Copy link
Contributor

From testing v0.0.4 I found that there are some missing type annotations. This PR adds type annotations to quaxify, makes _Quaxify generic wrt the function, and adds a few miscellaneous types.

@nstarman
Copy link
Contributor Author

nstarman commented Jul 17, 2024

Test failure is from the pre-commit pyright being out of date. This passes when pyright is updated!

Copy link
Owner

@patrick-kidger patrick-kidger left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -38,7 +38,6 @@ addopts = "--jaxtyping-packages=quax,beartype.beartype(conf=beartype.BeartypeCon
[tool.ruff.lint]
select = ["E", "F", "I001"]
ignore = ["E402", "E721", "E731", "E741", "F722"]
ignore-init-module-imports = true
Copy link
Owner

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Newer ruff doesn't require it. It surfaces a warning saying that it's being deprecated.

@patrick-kidger
Copy link
Owner

Test failure is from the pre-commit pyright being out of date. This passes when pyright is updated!

Update pyright in this MR as well? :)

@nstarman
Copy link
Contributor Author

nstarman commented Jul 18, 2024

Update pyright in this MR as well? :)

I actually don't know how pre-commit is selecting the pyright version. Is there a reason this repo runs pre-commit manually, versus using pre-commit.ci ?

@patrick-kidger
Copy link
Owner

The version is specified in .pre-commit-config.yaml. The pre-commit command itself is ran during tests.

I guess I'd spin that around, why use an external service for something that is already simple enough to do manually?

@nstarman
Copy link
Contributor Author

nstarman commented Jul 20, 2024

The version is specified in .pre-commit-config.yaml

Oops, duh. I git-exclude my pre-commit-configs once I lock in the set of hooks so that I never think about it and let pre-commit.ci wholly manage upgrading the hooks. So it wasn't showing being changed in this PR for git add -A!

external service for something that is already simple enough to do manually?

Personally I (and astropy) do it for

  1. Automatic upgrading of hooks.
  2. Option to comment pre-commit.ci autofix for contributors that don't use pre-commit.

@nstarman nstarman requested a review from patrick-kidger July 20, 2024 19:10
Signed-off-by: nstarman <nstarman@users.noreply.github.com>
@patrick-kidger patrick-kidger merged commit 9b27fc0 into patrick-kidger:main Jul 21, 2024
2 checks passed
@patrick-kidger
Copy link
Owner

Oh, the autofixing PRs is quite nice. Indeed sometimes we do find new contributors who haven't used pre-commit locally. (Although in my experience pretty much everyone uses pre-commit, so this doesn't come up super frequently.)

Anyway, merged! This PR looks good to me. If this now has everything you want I'm happy to do a new versioned release as well. (Or a little later if you find there's some other tweaks still needed.)

@nstarman nstarman deleted the add-types branch September 17, 2024 04:38
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants