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] resolve shellcheck errors in .ci/test_r_packages.sh #6501

Closed
wants to merge 28 commits into from
Closed

[ci] resolve shellcheck errors in .ci/test_r_packages.sh #6501

wants to merge 28 commits into from

Conversation

Kunal-Singh-Dadhwal
Copy link
Contributor

@Kunal-Singh-Dadhwal Kunal-Singh-Dadhwal commented Jun 21, 2024

Contributes to #6498

So here i rectified the errors in test_r_packages.sh and also checked it with shellcheck

@Kunal-Singh-Dadhwal
Copy link
Contributor Author

@microsoft-github-policy-service agree

@Kunal-Singh-Dadhwal Kunal-Singh-Dadhwal changed the title Rectified errors in test_r_packages.sh Resolved errors in .ci/test_r_packages.sh Jun 21, 2024
@Kunal-Singh-Dadhwal
Copy link
Contributor Author

Kunal-Singh-Dadhwal commented Jun 21, 2024 via email

@jameslamb jameslamb changed the title Resolved errors in .ci/test_r_packages.sh [ci] resolve shellcheck errors in .ci/test_r_packages.sh Jun 21, 2024
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 interest in LightGBM!

I've added a link to the issue I suspect you found this work from (#6498). Please do that in the future, it helps everyone involved to understand why you're proposing these changes.

Please click through the links to CI jobs and fix the failures reported there. You can @ me help if you need it.

Also... in the future, when you contribute to open source projects on GitHub, don't use the master / main branch of your fork. Create a new branch with your changes. Now that there have been some changes merged to master here, when you merge those into your fork's master branch, it'll have a different history than this repo's master branch, which will cause problems if you create another pull request in the future.

.ci/test_r_package.sh Outdated Show resolved Hide resolved
.ci/test_r_package.sh Outdated Show resolved Hide resolved
.ci/test_r_package.sh Outdated Show resolved Hide resolved
.ci/test_r_package.sh Outdated Show resolved Hide resolved
Co-authored-by: James Lamb <jaylamb20@gmail.com>
@jameslamb
Copy link
Collaborator

Please also merge the latest state of master into this branch.

Kunal-Singh-Dadhwal and others added 2 commits June 21, 2024 21:55
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
@Kunal-Singh-Dadhwal
Copy link
Contributor Author

Actually this was my first open source contribution. Thank you for your feedback and guidance! I appreciate your insights and will make sure to follow your suggestions for future contributions.

@Kunal-Singh-Dadhwal
Copy link
Contributor Author

Kunal-Singh-Dadhwal commented Jun 21, 2024

Please also merge the latest state of master into this branch

Should i make my own branch and push these changes

@jameslamb
Copy link
Collaborator

this was my first open source contribution.

Oh great, thanks for choosing LightGBM for that! We're happy to have you.

In the future, when you make a contribution (especially to a project you've never worked on before), please provide more information in the pull request title / description than "resolve errors". For example, can you please update the description here with the full text of all the errors you're trying to address?

That'd help reviewers to understand specifically what you are attempting to fix, and would help answer the important question "why should we merge this? how does this improve the project?".

@jameslamb
Copy link
Collaborator

Should i make my own branch and push these changes

Not for this PR. After this PR is merged, you should do the following if you intend to contribute here again:

  • delete your LigthGBM fork here on GitHub
  • recreate it

And then in the future if you contribute here, you should work from a new branch on your fork for every pull request, like this:

git checkout -b ci/shellcheck

@jameslamb
Copy link
Collaborator

Can you please merge in master again and resolve conflicts?

We had an in-progress pull request (#6497) that touched some of the same lines in in .ci/test_r_package.sh as this one does, and I've just merged that.

.ci/test_r_package.sh Outdated Show resolved Hide resolved
.ci/test_r_package.sh Outdated Show resolved Hide resolved
.ci/test_r_package.sh Outdated Show resolved Hide resolved
.ci/test_r_package.sh Outdated Show resolved Hide resolved
.ci/test_r_package.sh Outdated Show resolved Hide resolved
.ci/test_r_package.sh Outdated Show resolved Hide resolved
Co-authored-by: James Lamb <jaylamb20@gmail.com>
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.

This is getting closer!

Please look through the diff at https://github.com/microsoft/LightGBM/pull/6501/files to identify whitespace-only changes, and please revert those.

I see two. This one on line 151 where you've added back these spaces I asked you to remove:

image

And this one, where you've removed an entire line:

image

If you're unsure how to find those extra space characters, consider changing the settings in your editor to display whitespace characters.

.ci/test_r_package.sh Outdated Show resolved Hide resolved
@Kunal-Singh-Dadhwal
Copy link
Contributor Author

@jameslamb removed whitespaces

@jameslamb
Copy link
Collaborator

Please put this line back:

image

@jameslamb
Copy link
Collaborator

Thanks, but it seems that the most recent commit you pushed re-introduced those extra space characters. I think maybe you aren't aware of how to identify space-only characters when adding text files.

No problem, in some editors they're hidden! To get this PR merged, I just pushed a commit removing those. You can view it here: e1c71cc

You may want to turn on display of whitespace characters in your editor. Here's what this file looks like to me in VS Code (note that dots for spaces):

image

@jameslamb jameslamb self-requested a review June 22, 2024 02:59
@Kunal-Singh-Dadhwal
Copy link
Contributor Author

Will check thoroughly the next time I make any contribution. Thanks for your patience and guidance

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 this!

I've pushed a few commits here to finish this.

We'd love your help with more shellcheck things to make progress on #6498 if it interests you. If you do submit future pull requests ("PR"), please consider the suggestions I've given in this review:

@Kunal-Singh-Dadhwal Kunal-Singh-Dadhwal closed this by deleting the head repository Jun 22, 2024
@jameslamb
Copy link
Collaborator

@Kunal-Singh-Dadhwal it looks like you deleted your fork before this was merged, so now your work won't actually be added to LightGBM. I strongly suspect that was a mistake.

If it was and you are still interested in contributing these changes, please open a new pull request.

@Kunal-Singh-Dadhwal
Copy link
Contributor Author

@jameslamb Yes it was a mistake Sorry ,I would make another PR by today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants