Skip to content

Update CONTRIBUTING.md#3164

Merged
vadi2 merged 1 commit intodevelopmentfrom
rule-no-mega-prs
Oct 31, 2019
Merged

Update CONTRIBUTING.md#3164
vadi2 merged 1 commit intodevelopmentfrom
rule-no-mega-prs

Conversation

@vadi2
Copy link
Copy Markdown
Member

@vadi2 vadi2 commented Oct 15, 2019

Brief overview of PR changes/additions

Ban mega PRs.

Motivation for adding to Mudlet

Very bad experiences with them.

Other info (issues closed, discussion etc)

We've always frowned upon them, but tolerated them, and honestly it's not working out. It's a frustrating and disappointing experience for everybody involved: the PR author, the core team, and the players.

@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Oct 15, 2019

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

Copy link
Copy Markdown
Contributor

@Kebap Kebap left a comment

Choose a reason for hiding this comment

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

Yeah, let's roll that way

Copy link
Copy Markdown
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

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

👎 No - lets not forget that making an omelette requires you at some point to break some eggs. Some changes cannot reasonably be introduced in a piecemeal fashion - yes I accept that in an ideal situation each change can be broken down into smaller chunks - but some things do make fundamental changes internally and they cannot easily be done bit-by-bit - for instance does #3131 or #3132 fall within the limits that are to be imposed?

Instead of:

Pull Requests that overhaul large pieces of functionality at once will not be accepted: through experience, they bring more pain than they are worth. Being really difficult to discuss, test, and reason about, they are banned.

I would substitute:
Pull Requests that overhaul large pieces of functionality at once will be very hard for us to accept: through experience, they bring much pain that can be more than they are really worth. Being difficult to discuss, test, and reason about, they are not going to be welcomed and may end up being abandoned and unused.

@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Oct 16, 2019

#3131 falls under the proposed rule - so I'll have to break it up into smaller PRs, and I definitely can. Not everything has to go in at once, there are underlying changes I did that I can put in at first, get merged, and then make use of them in later PRs.

#3132 - like other rules in Contributing, I think we should go easy on external contributors, to encourage contributions. There's a limit of course to what we can accomodate but all in all, we can hold ourselves to a higher standard easier.

Copy link
Copy Markdown
Member

@keneanung keneanung left a comment

Choose a reason for hiding this comment

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

Yes, please. They are hard to review and take much longer. Additionally, the change requests are usually much more devastating due to the sheer amount.

@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Oct 29, 2019

@SlySven ?

@SlySven
Copy link
Copy Markdown
Member

SlySven commented Oct 30, 2019

Is my suggested alternative going to fly or not? I am reluctant to announce a "We are never going to accept something" when there is a possibility that "It might be accepted but you don't want to bet on that" is closer to the mark...

@vadi2
Copy link
Copy Markdown
Member Author

vadi2 commented Oct 30, 2019

That's what we had before... it didn't work! So yes, now it has to be split up.

@SlySven SlySven self-requested a review October 30, 2019 21:07
@SlySven SlySven dismissed their stale review October 30, 2019 21:10

To go along with the crowd and not hold up approval - even though I don't wholly agree with it.

Copy link
Copy Markdown
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

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

😶

@vadi2 vadi2 merged commit 9f8b82b into development Oct 31, 2019
@vadi2 vadi2 deleted the rule-no-mega-prs branch October 31, 2019 03:01
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.

5 participants