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 some return type hints #834

Merged
merged 2 commits into from
Oct 27, 2023
Merged

add some return type hints #834

merged 2 commits into from
Oct 27, 2023

Conversation

jameslamb
Copy link
Contributor

This PR proposes adding return type hints to a few methods that were missing them. This improves the ability of type checkers like mypy to catch type-checking issues like "this method can return None but its output is being used unconditionally as if it were not None".

Question for Reviewers

These are just a few things I saw while starting to familiarize myself with this codebase. There are other methods missing return type hints... would you support additional PRs like this adding return type hints?

I saw mixed use of them throughout the project, but assume they're desirable since mypy is run in CI:

- name: Run Mypy
run: mypy

If this type of contribution is annoying and you aren't open to more such PRs, please do tell me and I won't propose more.

Thanks very much for your time and consideration!

@emeli-dral emeli-dral requested a review from mike0sv October 26, 2023 10:04
@emeli-dral
Copy link
Contributor

Hi @jameslamb ,
Thank for helping out! This type of contribution is very welcomed although we do not get it often :)

I see that there are some failed tests. I believe it has happened because we had issues in the main branch which have already been resolved. Could you please merge main into your branch? I think this should fix tests.

@jameslamb
Copy link
Contributor Author

This type of contribution is very welcomed

Ok thanks! I'll be making more like it then 😁

Could you please merge main into your branch

Just did that and updated this branch. I just merged instead of rebasing because it looks like this project squashes commits on merge, but happy to squash/rebase here if that's what yall prefer.

@emeli-dral emeli-dral merged commit d43d330 into evidentlyai:main Oct 27, 2023
@jameslamb jameslamb deleted the ci/mypy branch October 27, 2023 16:49
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