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

[python] Add type hints to helpers/parameter_generator.py #4474

Merged
merged 18 commits into from
Jul 24, 2021

Conversation

sagnik1511
Copy link
Contributor

@sagnik1511 sagnik1511 commented Jul 15, 2021

contributes to #3756

Added type hints on helpers/parameter_generator.py

@ghost
Copy link

ghost commented Jul 15, 2021

CLA assistant check
All CLA requirements met.

@jameslamb jameslamb changed the title [helpers] Add type hints to the helpers/parameter_generator.py [python] Add type hints to the helpers/parameter_generator.py Jul 16, 2021
@jameslamb jameslamb requested review from jameslamb and removed request for guolinke July 16, 2021 04:28
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for your help! Please see the suggestions from my first review. You can accept them directly in the browser by clicking "add to batch" here on GitHub, then "commit suggestions".

These suggestions could be summarized by these three practices:

  • when using types which can contain instances of other types, add type hints for those contents. For example, a list of tuples of 3 integers should be List[Tuple[int, int, int]], not List
  • only use Optional for something which can be None
  • give functions which explicitly return None or implicitly return it by not having any return statements the return hint -> None

helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
@jameslamb jameslamb changed the title [python] Add type hints to the helpers/parameter_generator.py [python] Add type hints to helpers/parameter_generator.py Jul 16, 2021
sagnik1511 and others added 10 commits July 16, 2021 10:42
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
@sagnik1511
Copy link
Contributor Author

Thanks, @jameslamb for reviewing, I understood that the codes should be more specific and less coded. Thank You, sir.

@jameslamb jameslamb self-requested a review July 16, 2021 14:27
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks very much!

I found some additional issues by running mypy helpers/parameter_generator.py.

helpers/parameter_generator.py:31: error: Need type annotation for 'cur_info' (hint: "cur_info: Dict[<type>, <type>] = ...")
helpers/parameter_generator.py:33: error: Need type annotation for 'member_infos' (hint: "member_infos: List[<type>] = ...")
helpers/parameter_generator.py:126: error: Invalid tuple index type (actual type "str", expected type "Union[int, slice]")
helpers/parameter_generator.py:127: error: Invalid tuple index type (actual type "str", expected type "Union[int, slice]")
helpers/parameter_generator.py:291: error: Argument 1 to "get_names" has incompatible type "List[Dict[str, Any]]"; expected "List[List[Dict[str, Any]]]"
helpers/parameter_generator.py:292: error: Argument 1 to "get_alias" has incompatible type "List[Dict[str, Any]]"; expected "List[List[Tuple[Dict[str, Any]]]]"
helpers/parameter_generator.py:328: error: Invalid index type "str" for "str"; expected type "Union[int, slice]"
helpers/parameter_generator.py:329: error: Invalid index type "str" for "str"; expected type "Union[int, slice]"
helpers/parameter_generator.py:330: error: Need type annotation for 'checks' (hint: "checks: List[<type>] = ...")
helpers/parameter_generator.py:332: error: Incompatible types in assignment (expression has type "str", variable has type "List[Any]")
helpers/parameter_generator.py:332: error: Invalid index type "str" for "str"; expected type "Union[int, slice]"
helpers/parameter_generator.py:343: error: Invalid index type "str" for "str"; expected type "Union[int, slice]"
helpers/parameter_generator.py:344: error: Invalid index type "str" for "str"; expected type "Union[int, slice]"
helpers/parameter_generator.py:359: error: Incompatible return value type (got "Tuple[List[Tuple[str, int]], List[Dict[str, Any]]]", expected "Tuple[List[Tuple[str, int]], List[List[Dict[str, Any]]]]")
Found 14 errors in 1 file (checked 1 source file)

I pushed sagnik1511@11ed4b3 to this branch to fix them.

@sagnik1511
Copy link
Contributor Author

sagnik1511 commented Jul 16, 2021

Thank You @jameslamb, for that.
Can I try to update other files?

@jameslamb
Copy link
Collaborator

Can I try to update other files?

Sure, please open a new pull request if you'd like to update a new file. Please consider the suggestion in #4474 (review) when you do that.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

I think we can be even more precise and use List instead of Any:

helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
helpers/parameter_generator.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@sagnik1511
Copy link
Contributor Author

sagnik1511 commented Jul 19, 2021

@StrikerRUS, @jameslamb, thank you for letting me collaborate. Please, do merge this PR :)

@jameslamb
Copy link
Collaborator

Sorry for the delay in merging, thanks again for the help!

@jameslamb jameslamb merged commit 55c3815 into microsoft:master Jul 24, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants