Formally addressing the way PS goes about pull requests

[panpawn]

Pull Requests These are a major part of the PS development culture. However, in my opinion, there's a huge issue with the way that some pull requests are handled in that they sometimes are merged into the master branch before they are necessarily ready for the production server or to otherwise go live.

Some people might think that there's nothing wrong with prematurely merged pull requests, and that we typically do a lot to prevent that, and generally I would agree. However, this is not always the case; sometimes, we don't seem to do the best that we could to prevent this from happening, based on my observations.

When a pull request is prematurely merged due to a lack of review, this can leave bugs or even crashes in the current version of PS, which otherwise could of been prevented. This has happened several times before, but we should try to do the best to prevent this to our ability.

When we don't try to do the best we can to prevent this, it can cause either other developers or the developer who worked on the pull request to go back and try to find how to work on a fix, which often times takes that developer away from working on whatever it was they were previously.

In comparison, several other projects on GitHub will make it so that a pull request much get X number of approvals before being merged, or alternatively, must be open for a minimum period of time. I believe that something to this effect would help the current issue of some pull requests just not being open long enough.

So, all of this comes down to several differing meanings of what a "pull request" is among the development team.

To me, a pull request are changes that someone is proposing to the project, but not necessarily the best way of going about those changes. For this reason, I am a firm believer that in general, a pull request should be reviewed, preferably by several people if several people are currently working on that part of PS that is relating to said pull request.

A big part of being open-source is collaboration, and if a pull request isn't open long enough for people to actually help the collaboration of that change, that is straying from what I think a pull request is, which is probably why I feel this issue is important.

It has come to my attention that owners of repositories can actually set a minimum number of approvals before a PR can actually be merged, but I think we could just do a "thumb up" kind of a thing or something on the actual PR to simplify things or something, though.

Also, somewhat unrelated to this but worthy to mention: I think we should be holding collaborators to the same contributing standards as everyone else. Namely, this has to do with commit name standards specifically, which I have seen several instances where collaborators don't follow what is outlined in CONTRIBUTING.md.

The meaning of this issue is to think about the things that I've talked about, and to see if there is actually room for improvement in these areas, and if so, to give helpful feedback and your thoughts on the topics that I've outlined above.

With that being said, what does everyone else think?

[panpawn]

@bumbadadabum - if you could do more than just thumb it down, and explain why, that would much be appreciated for the sake of discussion.

[Zarel]

He's annoyed at you and doing it because you do it to his posts a lot.

Honestly, I don't really see a need for thumbs-down as a reaction in general... makes it hard for people to get along, I think, and if you have criticisms you can comment like normal.

[panpawn]

Which is why I typically also will comment explaining why I thumbed down instead of just thumbing down...

[Zarel]

Yeah, in which case the thumbs-down doesn't really add anything the comment doesn't already say, except make it harder to be friends with you.

[Asheviere]

Honestly though I mostly just wanted to say I disagree and didn't feel like making a post while also doing the thing @Zarel pointed out, not primarily that though. I just think this is unnecessary. We're a small enough team to handle this without any real policy. I feel like this would only inconvenience stuff and not change and fix anything.

[panpawn]

Yeah, in which case the thumbs-down doesn't really add anything the comment doesn't already say, except make it harder to be friends with you.

I agree that giving constructive criticism is better than just thumbing down, but perhaps that is a discussion for another time since it isn't relevant to the topic of this issue.

We're a small enough team to handle this without any real policy. I feel like this would only inconvenience stuff and not change and fix anything.

I want to make note here that I'm not necessarily proposing a standard that must be followed in every single situation, but rather, as a general rule of thumb.

To say that this wouldn't help is also to deny that this would have ever helped the stability of the Main server, which I believe is false based upon several cases that I've personally experienced.

Having a minimum time as a rule of thumb would be helpful because we're a smaller development team, not vis versa. Larger development teams that have a lot more users are more likely to actually get feedback in a smaller period of time because they have more developers to review pull requests. We have less, which is why it should be open longer to get as much feedback as possible in most instances.

[Morfent]

It has come to my attention that owners of repositories can actually set a minimum number of approvals before a PR can actually be merged, but I think we could just do a "thumb up" kind of a thing or something on the actual PR to simplify things or something, though.

I don't know if this would work because of the huge variance in the number of people that have modified certain parts of PS. Some parts of PS have only one or two maintainers that have ever modified it before and are still active, but it wouldn't make sense to only require one or two thumbs up for changes to files like chat-commands.js or any one of the battle simulator files, where the extra reassurance would be useful. Regardless, the crash logger and /hotpatch already mitigate the problems this tries to fix fairly well on their own.

[panpawn]

I don't know if this would work because of the huge variance in the number of people that have modified certain parts of PS.

I talked about alternative ideas such as having a minimum time a PR must be open for, too.

Regardless, the crash logger and /hotpatch already mitigate the problems this tries to fix fairly well on their own.

key word: "mitigate" (also it doesn't "mitigate" things that aren't hotpatchable, of which, a lot of PS isn't)

edit; hotpatching also is somewhat resource intensive, and on a big group project like PS, hotpatching might not be possible in all instances where it is required to patch a but that outhrwise, again, could have been prevented if a pull request had been open for a longer period of time.

The problems this "tries to fix" is before crashing the server, issues are properly addressed before being merged and causing problems on the production server.

The ideology that we should wait for crashes to come and accept premature pull requests is to accept a state of development that the master branch isn't always as stable as it could be. To me, this is unacceptable.

[Zarel]

My short answer is "no" but I've been putting off writing a long answer.

[TheImmortal]

You're making a simple matter complicated. Pull requests should not be blindly accepted. That's it. We don't need to scrutinize them with a magnifying glass and delay features being added to the server. As much as you would review it, you don't know for certain how its going to run on the live server.

I actually doubt Zarel accepts pull requests without reviewing them at all, which makes this issue non-existent.

[Zarel]

Anyway, I review nearly all pullreqs I merge, but sometimes it's just a cursory skim. The main exception is pullreqs to chat plugins which I often just merge blindly. I also sometimes merge tour changes blindly, if they're from someone I trust.

I think something breaking every once in a while isn't a huge deal. We react to issues quickly enough for it to be good enough; I've always fixed things or reverted them when issues come up.

That "BETA" is there for a reason, and in general I think most people would appreciate faster updates than stability and bureaucracy.

Fast merging also makes developers happier, since they like to see their changes in PS faster.

[Zarel]

I think that mostly covers it: my answer is, I don't currently see a need for changes.

[panpawn]

Pull requests should not be blindly accepted. That's it.

I agree.

The main exception is pullreqs to chat plugins which I often just merge blindly.