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

[ci] for autogenerating docs #2076

Merged
merged 6 commits into from May 25, 2019
Merged

[ci] for autogenerating docs #2076

merged 6 commits into from May 25, 2019

Conversation

nabokovas
Copy link
Contributor

Comments were reworked there for easier autogenerating documentation.
No changes for the functions.
No description for LGBM_NetworkInitWithFunctions.

@guolinke guolinke requested a review from StrikerRUS April 3, 2019 08:39
@StrikerRUS
Copy link
Collaborator

Awesome!

Great addition for #2059!

@nabokovas Please fix the conflict before I start the review.

@guolinke I suppose this PR should be merged after #2059, right?

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.

@nabokovas Thanks for your huge work!
I left some comments about Dataset API below.

Also please use the same description for the handle param across the file: either Handle of dataset or An instance of dataset.

include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
@guolinke
Copy link
Collaborator

guolinke commented Apr 4, 2019

@StrikerRUS yeah, I will take a look for #2059

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.

@nabokovas Below is the second round of the review.
Also, please use either File name or Filename in descriptions across this file.

include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h 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.

Another couple of comments:

include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h 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.

@nabokovas Here is the last part of review.

Also, please replace @ symbols back to canonical \ ones. This should reduce PR's diff significantly and \ are already used all over the project.

include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
include/LightGBM/c_api.h Outdated Show resolved Hide resolved
@guolinke
Copy link
Collaborator

@StrikerRUS maybe you can fix these minor issues directly and then this PR could be merged.

@StrikerRUS
Copy link
Collaborator

@guolinke Sure, I think I can take this PR over in case of author's further inactivity.

@StrikerRUS
Copy link
Collaborator

@guolinke BTW, while we are speaking about abandoned PRs, can you please share your opinion in #1864?

@StrikerRUS
Copy link
Collaborator

ping @nabokovas
Are you with us?

@nabokovas
Copy link
Contributor Author

I am sorry for the really long delay

@StrikerRUS
Copy link
Collaborator

@nabokovas No problem!
Thanks a lot for your patience and valuable contribution!

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!

@StrikerRUS StrikerRUS merged commit 19de2be into microsoft:master May 25, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants