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

Adding type hinting tests #9319

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

michael-gendy-mention-me
Copy link
Contributor

As part of this issue, we want to add mypy support to our tests. This PR will be type hinting a range of tests for the Python package

@michael-gendy-mention-me
Copy link
Contributor Author

@trivialfis I've started type hinting for the tests here, beginning with test_basic.py. I'm happy to continue for remaining tests, but I'm not sure how much value adding type hinting for the tests is generating. Do you see this as important and should I therefore continue with more files?

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Thank you for the work on type hint @michael-gendy-mention-me !

I'm happy to continue for remaining tests, but I'm not sure how much value adding type hinting for the tests is generating. Do you see this as important and should I therefore continue with more files?

Indeed, I believe it's essential. By having type hints in tests, we can check the type-hint of the xgboost library from a user's perspective thoroughly.

Based on this, my priority for type hint implementation would be:

  • Core module.
  • Scripts in demo/guide-python.
  • Other scripts like dask and survival training in demo.
  • Tests.

I put demo over tests just to add value as soon as possible. The type hint might make some of the demonstration scripts easier to read. Others can freely choose what they want to work on.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Please add the file to:

Install mypy, black, and isort. Run the script in the project root. Feel free to disable --pylint and let the CI run it for time saving.


# save, assert exists, remove file
binary_path = Path("dtrain.bin")
binary_path: Path = Path("dtrain.bin")
Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with these changes. But for the ease of working on type hints and consistency with the rest of the library, trivially deducible types don't need annotation.

@@ -16,32 +18,31 @@
class TestBasic:
def test_compat(self):
Copy link
Member

Choose a reason for hiding this comment

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

mypy doesn't check the function if there's no annotation in the signature.

Suggested change
def test_compat(self):
def test_compat(self) -> None:

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

2 participants