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

Stale set up #2101

Merged
merged 1 commit into from
Dec 14, 2017
Merged

Stale set up #2101

merged 1 commit into from
Dec 14, 2017

Conversation

SergioRAgostinho
Copy link
Member

@SergioRAgostinho SergioRAgostinho commented Nov 23, 2017

I recently requested @jspricke to set up Stale. My original idea was to have it automatically tag PRs which have no updates passed a certain time and remove them at some point, so I can have an easier job navigating the PR list. Jochen already set it up on PCL's repo so now is the time do decide exactly what type of behavior we want from it.

I configured it already for my (revised) needs but it would be good if you could also voice you opinions on what you consider to be the desired behavior.

Jochen already expressed his opinion on an email exchange we had and I quote.

Sérgio Agostinho [2017-11-22 17:07]:
I want to configure it to close PRs which we have no feedback in more than 90 days.

Honestly, I don't think that's the right way to handle PRs. PRs means someone had an issue, looked into the code and wants to give something back. I know it's bad to leave these contributions lying around for a long time, but they are still valuable. I'm aware of a number of PRs a lot of people use, but no one took the time to integrate. Or just as well, people finding a patch, improve it and rebase it for merging.

On the other hand, issues would be a different point and are mostly less valuable in my point of view. If you find a mayor bug, it may make sense to open an issue, but normally I wouldn't expect someone to work on a bug just because I opened an issue.

I know it's bad to don't give feedback to contributors, but that's the reality in open source and as long as it's all voluntary, it will not change. Closing PRs would just be more frustrating and give the impression that we don't want contributions.

@taketwo
Copy link
Member

taketwo commented Nov 26, 2017

I think we should not automatically close staled PRs or issues. So your current config is fine for me.

@UnaNancyOwen
Copy link
Member

I think so too. We should not automatically close old pull requests and issues.
These are important contributions. Our resources are limited, but I think we should not refuse them.

@SergioRAgostinho
Copy link
Member Author

If you're all ok with the configuration please give the final go accepting the changes proposed.

@SergioRAgostinho
Copy link
Member Author

Ping @jspricke. The final check and merge.

@jspricke
Copy link
Member

I think the comment should mention that we value the contribution but are just overloaded with work and will try to look over it at some point in the future. Otherwise fine with me.

@SergioRAgostinho
Copy link
Member Author

That can't happen. PRs that need review are tagged with "status: needs review" and that tag is exempt from the stale check.

@jspricke
Copy link
Member

But what should be the message of the comment then? It will be send to the PR author, right?

@SergioRAgostinho
Copy link
Member Author

SergioRAgostinho commented Nov 30, 2017

The tag is only applied when there's no feedback from the author or when the PR is frozen due to some upstream situation which needs fixing. In the second scenario I still try to provide the review regardless, so that we have everything in place, ready for the upstream fix.

The stale tag and notifications will only affect authors who stopped giving feedback after 60 days, either because we asked some questions and are still waiting for answers or because most work is happening under our radar and there's no new commits to the PR branch. A simple comment or commit from the author removes the tag automatically.

I think the message currently in place is fine. It notifies the author we haven't heard from him in 60 days and therefore we're marking his PR as stale.

I also assume the maintainers will receive the notification. So we have always means to double check if it's accurate. Edit: For instance it can mark a good time to check if an upstream fix was released.

@jspricke
Copy link
Member

I still don't get it, sorry. So the author should take action if the tag is applied? Then the message should state that imho

@SergioRAgostinho
Copy link
Member Author

Something like:

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

?

@taketwo
Copy link
Member

taketwo commented Nov 30, 2017

Well, such message describes what has happened and what can be done. But I think it will be more useful to encourage the contributor to come back and finish the work or at least answer our questions.

Something like "hey, we haven't heard from you for a while. we will mark this pr as stale for now, but we are looking forward to see you back to finish this pr and eventually merge it into mainstream pcl"

@SergioRAgostinho
Copy link
Member Author

But I think it will be more useful to encourage the contributor to come back and finish the work or at least answer our questions.

You reminded me of those marketing emails from Facebook, Duolingo. "Come back please. We miss you!" ^^

@taketwo
Copy link
Member

taketwo commented Nov 30, 2017

Haha, exactly :) The purpose of "status: stale" tag is to help us maintaining. But the purpose of the message is different, it is to bring the guy back.

@SergioRAgostinho
Copy link
Member Author

This pull request has been automatically marked as stale because it hasn't had
any activity in the past 60 days. Commenting or adding a new commit to the
pull request will revert this.

Come back whenever you have time. We look forward to your contribution.

?

@jspricke
Copy link
Member

Sounds good to me :).

@taketwo
Copy link
Member

taketwo commented Dec 1, 2017

👍

1 similar comment
@UnaNancyOwen
Copy link
Member

👍

@SergioRAgostinho
Copy link
Member Author

Should I merge this on my own?

@taketwo taketwo merged commit 9ff0254 into PointCloudLibrary:master Dec 14, 2017
@SergioRAgostinho
Copy link
Member Author

Nothing happened so far. That was kind of anti-climatic. :<

@jspricke
Copy link
Member

31 new mails in my inbox.. that escalated quickly :D

@taketwo
Copy link
Member

taketwo commented Dec 14, 2017

And something is wrong, because the bot closed a number of PRs, although he was not supposed to.

# Number of days of inactivity before an Issue or Pull Request becomes stale
daysUntilStale: 60
# Number of days of inactivity before a stale Issue or Pull Request is closed
# daysUntilClose: 7
Copy link
Member

Choose a reason for hiding this comment

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

Leaving this commented-out means that the default value is used, which is 7.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would try to set it either false or then to a ridiculously big number 2^32 perhaps.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

So we should rather go for a big number. And perhaps file a feature request for the bot developers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll handle the reopen of all PRs once that is changed.

Copy link
Member

Choose a reason for hiding this comment

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

Feature request: probot/stale#79

@jspricke
Copy link
Member

I would propose to revert this until it is fixed and reopen the PRs. What do you think?

@SergioRAgostinho
Copy link
Member Author

SergioRAgostinho commented Dec 18, 2017 via email

@jspricke
Copy link
Member

#2162 I will reopen the PRs once it's merged.

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

Successfully merging this pull request may close these issues.

4 participants