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-package] prefix several internal functions with _ #5545

Merged
merged 10 commits into from
Nov 4, 2022

Conversation

Madnex
Copy link
Contributor

@Madnex Madnex commented Oct 17, 2022

Contributes to #5313, by prefixing the following internal functions in lightgbm.basic with _:

  • cfloat32_array_to_numpy()
  • cfloat64_array_to_numpy()
  • cint32_array_to_numpy()
  • cint64_array_to_numpy()
  • c_str()
  • is_1d_list()
  • list_to_1d_numpy()
  • cfloat32_array_to_numpy()

@Madnex Madnex changed the title Prefixing multiple [python-package] prefixing multiple internal functions Oct 17, 2022
@Madnex
Copy link
Contributor Author

Madnex commented Oct 17, 2022

@microsoft-github-policy-service agree

@jameslamb jameslamb changed the title [python-package] prefixing multiple internal functions [python-package] prefix several internal functions with _ Oct 18, 2022
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 for your help with this! Please see the one suggestion I made.

I also made two changes to this PR:

  • listed the specific changes object names in the description (since the PR title saying "multiple" isn't enough to indicate which objects you intended to change)
  • made the PR title slightly more specific so it will be a bit easier for people reading the release notes to understand what this PR did

Also, I noticed that you did one commit per object. That's totally fine if that helped you to organize your thoughts, but if you worked that way for the benefit of LightGBM, I want to let you know you don't need to do that extra work here in the future. This project uses squash merging, which means that all of the changes from a PR are combined into a single commit on master when the PR is merged.

tests/python_package_test/test_basic.py Outdated Show resolved Hide resolved
@Madnex
Copy link
Contributor Author

Madnex commented Oct 19, 2022

Thanks for the hints @jameslamb! I will arrange the future changes for this issue than in less commits and mention the changed definitions explicitly.

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, thanks!

@jameslamb jameslamb self-requested a review October 29, 2022 22:39
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! We'll merge this once we fix the project's CI issues (unrelated to your PR).

@jameslamb
Copy link
Collaborator

@Madnex sorry for the delay! We've now fixed the project's CI issues.

Sorry for the inconvenience, but could you please merge the latest changes from master into that branch? Once you do that and CI runs successfully, I'd be happy to merge this.

Thanks so much for your continued help with LightGBM!

@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 19, 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