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

Add lock bot #2815

Merged
merged 2 commits into from
Feb 27, 2020
Merged

Add lock bot #2815

merged 2 commits into from
Feb 27, 2020

Conversation

guolinke
Copy link
Collaborator

No description provided.

@guolinke
Copy link
Collaborator Author

BTW, there seems are some interesting bot for linting:
like https://github.com/marketplace/actions/autopep8
Maybe we can use github actions to auto format codes and documentation generation.
ping @StrikerRUS and @jameslamb

@StrikerRUS
Copy link
Collaborator

Why do we need locking old issues? Even after closing users may leave useful comments for others.

Speaking about pep8 bot, we already have configured CI linting job which tells contributors what should be fixed before PR will be approved.

LightGBM/.ci/test.sh

Lines 43 to 60 in 224b8b9

if [[ $TRAVIS == "true" ]] && [[ $TASK == "lint" ]]; then
conda install -q -y -n $CONDA_ENV \
pycodestyle \
pydocstyle \
r-stringi # stringi needs to be installed separate from r-lintr to avoid issues like 'unable to load shared object stringi.so'
conda install -q -y -n $CONDA_ENV \
-c conda-forge \
r-lintr>=2.0
pip install --user cpplint
echo "Linting Python code"
pycodestyle --ignore=E501,W503 --exclude=./compute,./.nuget . || exit -1
pydocstyle --convention=numpy --add-ignore=D105 --match-dir="^(?!^compute|test|example).*" --match="(?!^test_|setup).*\.py" . || exit -1
echo "Linting R code"
Rscript ${BUILD_DIRECTORY}/.ci/lint_r_code.R ${BUILD_DIRECTORY} || exit -1
echo "Linting C++ code"
cpplint --filter=-build/c++11,-build/include_subdir,-build/header_guard,-whitespace/line_length --recursive ./src ./include || exit 0
exit 0
fi

@guolinke
Copy link
Collaborator Author

@StrikerRUS
Actually, for the old issues, I think replying to them is easy to be ignored. As they are closed, I think they are solved at that time point. If the issues happen again, it may cause by different reasons.

For some important issues, like feature requests, bug, we can exclude them to be auto locked.
What do you think?

As for the bots, I want the "auto" methods, like auto format the codes and generate the documents, without we run some scripts or fix them one-by-one manually.

@StrikerRUS
Copy link
Collaborator

@guolinke I see your point! And I absolutely agree with you that in some cases locking old issues is required to prevent misleading comments. Refer for example to #1369. However, some other issues can benefit from new comments, e.g. #1597.

According to the present set of created labels, I think we should exclude blocking, bug, effectiveness, efficiency, feature-request, help-wanted, in progress.

As for the bots, I want the "auto" methods, like auto format the codes and generate the documents, without we run some scripts or fix them one-by-one manually.

I remember we had some discussion about docs auto-generation and inconvenience for contributors some time ago: #1511.
For auto-linting bots, where they will create PRs: into our repository after merging a PR with linting issues, or into contributor's fork when they submit a PR with linting issues?

@guolinke
Copy link
Collaborator Author

guolinke commented Feb 25, 2020

Refer to autopep8:

This action is designed to be used in conjunction with Create Pull Request. This will automatically create a pull request to merge fixes that autopep8 makes to python code in your repository.

It seems will create a new PR for the formatting.

Personally, I think auto-format the code in the original PR is better (new PR to contributors' repo is okay), but i don't know could it be supported or not.

@StrikerRUS
Copy link
Collaborator

@guolinke

Personally, I think auto-format the code in the original PR is better (new PR to contributors' repo is okay), but i don't know could it be supported or not.

In case of new PR, we will consciously merge "bad" code, and for some time the repo will be in undesirable state. Also, I'm not sure that auto-fixes are better than manual ones. Sometimes they may be over-strict or strange, and we won't be able to review them, only propose fixes for fixes in one more PR.

@jameslamb
Copy link
Collaborator

@guolinke

Personally, I think auto-format the code in the original PR is better (new PR to contributors' repo is okay), but i don't know could it be supported or not.

In case of new PR, we will consciously merge "bad" code, and for some time the repo will be in undesirable state. Also, I'm not sure that auto-fixes are better than manual ones. Sometimes they may be over-strict or strange, and we won't be able to review them, only propose fixes for fixes in one more PR.

^ I agree with this perspective. Every merge to master should leave the repo in a better state that it was as of the previous commit. And I agree that black might become yet another thing to manage, and add a new barrier to entry for contributors.

I am ok with having a bot lock old issues though! We as admins can always manually re-open issues that are locked if necessary, and GitHub's linking via comments means we won't lose the flow of conversation someone can easily open a new issue and say "I am experiencing something that looks similar to issue #whatever".

So to summarize, here is where I stand:

  • black or other auto-formatting bot: 👎
  • lock-bot: 👍

@guolinke
Copy link
Collaborator Author

@StrikerRUS @jameslamb thanks, got it. Let us only enable lock bot for now.

@guolinke guolinke merged commit 4fa3252 into master Feb 27, 2020
@guolinke guolinke deleted the lock-bot branch February 27, 2020 15:13
@StrikerRUS StrikerRUS mentioned this pull request Feb 29, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
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