We need a process for resolving PR's

How, what and why to code in BYOND.
Post Reply
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

We need a process for resolving PR's

Post by Oldman Robustin » #173788

PROBLEM

I have invested a significant amount of time/effort into learning our code, yet the frustration of trying to learn some eclectic code as someone who doesn't know how to code is completely dwarfed by my frustration with our PR system.

I have no problem walking away from a PR when its clear I misjudged player's attitudes, or someone comes along and does it better, or there is otherwise a majority disapproving of the process (see: security presence thread).

Yet time and again I'm sitting on PR's for 2-4 weeks when all substantive conversation and code changes are done within 3 days. I wouldn't object if there was a constant process of dialogue or improvement on a way to move forward, but that's simply not the case, the PR's become ghost towns and absolutely no progress is made for weeks while I fight off merge conflicts and become tangled in a web of derelict PR's that are increasingly difficult to track and manage. The obvious solution is to go to IRC, a fair request, #coderbus has been a good place for me to get help and advice on how to code here. However, as good as #coderbus is about actually helping with code, it's a goddamn dumpster fire when it comes to finding progress for your PR's.

Every time I join within 5 minutes, without fail, someone is mergebegging or testmerge begging their PR. At first I thought "begging" was just a meme, surely all I need to do is politely remind maintainers that my PR has been dead in the water for weeks and surely someone will get involved. Haha, nope. It really is begging, you have to start calling out specific people, PM'ing them, leaving them relentless memo's, and there's this suffocating atmosphere... like you just walked into a country club and while they let you in the front door, absolutely nobody will look at you or acknowledge your existence until you throw a shitfit and start yelling at specific people. For the established coders and maintainers it's mostly an "You scratch my back, I'll scratch yours" affair, when you have the power to merge PR's suddenly you have a lot of friends ready to curry favor in exchange for getting their shit merged. Public discussion barely even attempts to hide that it's a social network of ingratiation and politicking. Not only can maintainers offer great rewards to those they favor, they also possess great power to punish. I learned that lesson well when a sincere effort to update cult, requiring dozens of hours of learning code and testing locally, ended up getting explicitly grudgecoded against and turned into an absolute nightmare.

I mean let's compare. On December 10th the "Remove Oldcult, Add Newcult" PR was opened. There was significant opposition and playtests had demonstrated that the mode suffered from massive issues. Despite the opposition, including my own 100% accurate prediction of exactly which problems would plague it and why it would be awful (predicting that "pretty soon its off rotation for being unplayable"). The PR was merged 5 days later:

https://github.com/tgstation/-tg-station/pull/13552

Did this experience humble anyone? Were there any lessons learned? Did I get any kind of deference for being one of the only voices to correctly point out exactly why newcult would fail? Hahahahahah. The pattern repeats itself for other huge balance changes:

https://github.com/tgstation/-tg-station/pull/15250 (Regenerating blobbernauts that would single-handedly ruin blob from February to April)
https://github.com/tgstation/-tg-station/pull/15176 (EMP blob with EMP blobbernauts)

These, along with many other broken changes, were merged within 1-3 days and 3-4 comments. Meanwhile let's take a look at what happened when I made a PR that same maintainer disagreed with:

https://github.com/tgstation/-tg-station/pull/16778

Even at the end, the only reason it got merged was because Razharas yolomerged it for cluttering up Page 3, shortly before they lost maintainer status for yolomerging.

Yes I get it, you're volunteers, we should respect your contributions. But guess what, I'm a volunteer too. I suspect this entire operation would go more smoothly for ALL parties involved if there was an actual process for merging PR's.

SOLUTION

The process would look something like this:

1) Generally the sytem will follow a FIFO (First-in, First-Out) resolution process for PR's. When a PR is ready for review/merging it joins a list ranked from Oldest ---> Newest. Generally, only the oldest PR can be merged. Suddenly the maintainers have an incentive to review all PR's and not just the ones of friends or mergebeggers. It's a fair system that promotes an effective resolution process. If a PR makes it to the top of the list without getting reviewed, then it sits on standby, with priority for review, while other PR's may be merged ahead of it. As soon as the code review is finished, and if it's determined to be ready for merger, then it is first in-line to be resolved.

2) Bug fixes and certain high-priority changes would bypass the queue entirely and be entitled to immediate merger at the maintainer's discretion. Meme PR's and other obvious rejections can be yanked from the list at any time. Maintainers are free to give feedback or remove certain PR's from the queue for fixes/improvements even if its not the "oldest" PR. That way if a maintainer is well-versed in an area of code and recognizes an issue with a PR, they can put it back into the WIP list without burdening another maintainer with reviewing unfamiliar code. If a specific maintainer is sought to make a review before merging, then the PR can sit on "standby" at the top, letting other PR's get merged before it, but there would be some immediate resolution once the coder sought has given their opinion.

3) If the oldest PR in the list is controversial, unfinished, or requires further changes, the maintainer can apply the appropriate tag and it will be removed from the queue and would not be re-submitted until the issue is addressed. If the PR is controversial there must be some proposed resolution process before it can be removed from the queue. If the PR faces opposition from a significant majority, then it may simply be closed. The maintainer could also merge controversial PR's if its clear that recent commits have addressed concerns. If it is otherwise just controversial, it may be removed from the list but there must be a process for resolving the controversy. Ideally it would involve a proposed compromise, but other options could include a poll, testmerges, or simply a delay for further discussion. A further discussion tag would require a good-faith effort from other coders/maintainers to offer some additional feedback on how to resolve the controversy.

4) Even in the absence of controversy, balance or other major gameplay PR's should not be added to the queue, or otherwise removed from the queue if an adequate discussion period has not yet passed (probably 2-7+ days depending on the degree of opposition).

Is this system perfect? No of course not, but its still vastly superior to what we have now. If implemented properly it benefits EVERYONE, since maintainers will have an organized workflow that isn't just a hodgepodge of people screaming at them in IRC for reviews/merges/etc. and non-maintainers will have a clear idea of how to get their PR's merged. It also jives with our current tagging system. Ideally addressing the "oldest" complete PR at any time would be a painless process, once the code is reviewed then the maintainer has a straightforward decision-fork to guide them instead of the ambiguous mess we have now.
Last edited by Oldman Robustin on Wed May 04, 2016 7:21 pm, edited 2 times in total.
Image
User avatar
Bawhoppennn
Github User
Joined: Wed Jan 14, 2015 11:42 pm
Byond Username: Bawhoppennn
Github Username: Bawhoppen

Re: We need a process for resolving PR's

Post by Bawhoppennn » #173789

hahahahahahah robustin you're such a dank memer
I consider myself a /tg/station historian. If you're interested in the server history at all, feel free to ask me and I'll try and get you an answer! #ConcurForever

Image
<KorMobile> you're a hero

[21:20:53] <%oranges> Baw "has cute legs" hoppen
Image
DEAD: ADMIN(Owegno) says, "Nothing lewd happens in adminbus sadly."

[07:13:57] <Rockdtben> Keep in mind that I'm an extremely successful and wealthy male in his late twenties.

(F) DEAD: Professor DonkPocket says, "Admins preventchaos with good messages"

OOC: Pogoman122: Fun fact if someone trespasses on your kitchen just turn them into a nugget

Image

<+KorPhaeron> russians have no souls so magic enrages them
<+KorPhaeron> people who don't like rng are not from /tg/ and are likely redditors
ausops wrote:apart from this there is literally nothing more to say other than that this is the first thread in five years to have achieved something.
User avatar
TechnoAlchemist
Joined: Fri Nov 21, 2014 2:39 am
Byond Username: TechnoAlchemist

Re: We need a process for resolving PR's

Post by TechnoAlchemist » #173796

Maybe you should have more appealing PRS :^)

Also separation is absolute
onleavedontatme
Joined: Fri Mar 13, 2015 10:26 pm
Byond Username: KorPhaeron

Re: We need a process for resolving PR's

Post by onleavedontatme » #173801

3) If the oldest PR in the list is controversial
This is where it all breaks down and goes back to our current system.
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

Re: We need a process for resolving PR's

Post by Oldman Robustin » #173817

Kor wrote:
3) If the oldest PR in the list is controversial
This is where it all breaks down and goes back to our current system.
It's the weakest point of any system that will address the process, that doesn't mean you forsake all other improvements.

The important thing is that it gets addressed. That someone comes in and says "here's what you need to do to resolve this controversy" instead of having it just float through the PR pages for weeks without a single comment. If the oldest PR is controversial in the proposed system at least someone has to come in and inform the coder what they should do to move forward. Just the process of formalizing methods for resolving controversial PR's would be a huge improvement compared to "I don't fucking know just wait for someone to YOLOmerge it after a month maybe".

This Hivemind Link PR that's been sitting around for 35 days is a great example. 12 days ago I asked for someone to provide a path forward for the PR since it had already gone like a week without any changes or directions. I got feedback from other coders when I remade the PR, made their requested improvements, but still didn't get a path forward. Now after sufficiently plugging it in IRC you (Kor) come in and say I should've atomized it. The PR is 35 days old and substantively identical to the PR it was on Day 1. If you're actually telling me I HAVE to atomize it, why wasn't that something that could've been told to me a month ago? If you're not telling me I HAVE to atomize it (and I've written several times why I oppose it since all 3 changes are inter-related) then what's my path forward? A majority of people approve of the PR, it's been tested repeatedly, and it's a modest change with minor implications for balance... yet here we are 35 days later and I'm as clueless about what to do as I was on Day 3.

Aside from the controversial stuff, it would still be an improvement. Even modest changes and technical stuff can take a week or more for me to get merged, which is distracting at best and preventing me from making further improvements at worst. Kor I hope you recognize that you sit in the best position of the current system, I'd hoped you could better relate to how utterly shit the system is for others...

Plus if the worst case scenario is we end up back at our current system, why not give it a shot? I've poured a lot of energy into the cult changes and I'm confident that it's done a lot to improve the mode. I want to keep contributing but every time I look at a full page of open PR's and realize the only way they're going to turn purple is if I whore myself on IRC, my motivation is destroyed.
Image
User avatar
InsaneHyena
Joined: Thu Aug 27, 2015 9:13 pm
Byond Username: InsaneHyena
Github Username: InsaneHyena
Location: Russia

Re: We need a process for resolving PR's

Post by InsaneHyena » #173829

Maybe you should have more appealing PRS :^)
As a cult fan, Oldman might as well be the second coming of Jesus. And he has a point about newcult - nobody except Zilenan liked the changes, but coders shoved it into our throats anyway and then stubbornly refused to stop feeding us shit for how long?
Bring back papercult.

Image
User avatar
Hornygranny
Horny Police
Joined: Tue Apr 15, 2014 4:54 pm
Byond Username: Hornygranny

Re: We need a process for resolving PR's

Post by Hornygranny » #173843

Image
User avatar
Hornygranny
Horny Police
Joined: Tue Apr 15, 2014 4:54 pm
Byond Username: Hornygranny

Re: We need a process for resolving PR's

Post by Hornygranny » #173847

The fundamental problem here is that we have a small group of maintainers who will ultimately bear some responsibility for what they merge. It can be a tedious job when there's no politics involved, but some vocal members of the community have repeatedly shown the maintainers that if they merge anything controversial they will get flamed endlessly. What this means is that for any big PR a maintainer essentially has to volunteer to be shit on by the community even though it wasn't their idea, and a lot of the time they're simply not willing to do it.
Image
User avatar
oranges
Code Maintainer
Joined: Tue Apr 15, 2014 9:16 pm
Byond Username: Optimumtact
Github Username: optimumtact
Location: #CHATSHITGETBANGED

Re: We need a process for resolving PR's

Post by oranges » #173851

The only person besides cheridan who felt comfortable merging most pr's was raz and cheri fired them, so cheri has no one to blame but themself

ediit:Also robustin, what did you expect? You're a complete newbie to the repo and coding, making a huge change and expecting it to get merged with little to no review as a new developer is silly.

What you called politicking and ingratiation is just built up trust, which you too will achieve in time.

editedit:It doesn't help that one of the maintainers did grudgecode you though
User avatar
oranges
Code Maintainer
Joined: Tue Apr 15, 2014 9:16 pm
Byond Username: Optimumtact
Github Username: optimumtact
Location: #CHATSHITGETBANGED

Re: We need a process for resolving PR's

Post by oranges » #173853

The only thing I like about the new system is it will force maintainers to close pr's they dont' want, rather than leaving them to rot, 1) and 2) are the only decent points of your idea, the rest is just process that will slow things down.
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

Re: We need a process for resolving PR's

Post by Oldman Robustin » #173857

People go after the author of the hated shit, not the mergerer, at least in OOC/Deadchat. People shit on Goof all the time for his stuff, nobody can remember who actually added it.

This Saturday I woke up to find out that people could tablecraft infinite anything without any materials, by afternoon I realized the entire atmos system was broken (there was no adjacent atmos turf on 99% of tiles), and that evening we testmerged Goofmed, realized that people weren't dying even after 1,000 damage, etc... people will meme about coders but the actual impact it has on their reputation/credibility/authority has been nil. The average player has no clue who borked up their rounds, let alone which maintainer merged it. Only someone who does glorious self-branding like GOOF manages to have their name remembered among the playerbase.

@Oranges

It will slow things down for maintainers who are used to getting the express lane for their memes. I'd honestly expect the "ready" list to be nearly empty most of the time. If your PR is stalling, you'll know why, if someone wants to have their PR express laned, now they have an incentive to clear out some other, older ready-to-go PR's instead of ignoring them until they clutter up Page #3.

I've been patient, I don't think anyone can argue otherwise. I will sit on my hands for weeks if I'm getting feedback on my code. It's these long windows of silence and post-review waiting that has nothing to do with my (in)competence as a coder and everything to do with the fact that there's no incentive for a maintainer to pull the trigger on it.

Plus I'm only slightly resentful that I sit around all day with my local server, testing my PR's inside and out, only for the excuse to get pulled "we can't trust your crazy code Old Man!", while stuff like this Saturday with "trusted" coders getting their shit quickmerged with no testing and breaking the game happens a few times a week.
Last edited by Oldman Robustin on Thu May 05, 2016 4:48 am, edited 4 times in total.
Image
Scott
Github User
Joined: Fri Apr 18, 2014 1:50 pm
Byond Username: Xxnoob
Github Username: xxalpha

Re: We need a process for resolving PR's

Post by Scott » #173858

Maintainer power levels:

Cheridan: regular merges, once in a blue moon PR, headcoder/10
duncathan: fixed atmos, then broke atmos, alright/10
KorPhaeron: """Design""" lead
tkdrg: migrates to warmer lands every January, returns in Autumn
Joan: Merges things, fixes things, good/10
Remie: merges things, makes features, remie/10
anturk: does stuff, good/10
Jordie: changelogs
WJohn: label master, xeno enthusiast, only spriter, smooth/10
dorsisdwarf: purger of useless issues, over 800 issues open/10
phil235: fixer of everything, master cook, god/10
razharas: vodka, 40%/10


Robustin is right, some PRs just get ignored for months at a time, but those aren't very common cases.
User avatar
coiax
Joined: Sun Jan 31, 2016 10:45 am
Byond Username: Coiax

Re: We need a process for resolving PR's

Post by coiax » #173875

The longer a PR is open, the harder it is to get merged.

Devil Antag has been ready to merge multiple times, and when this has happened, people either find something else that's wrong with it, or nothing happens. The lack of movement on an admin spawn only side-antag is disconcerting.

Also, anytime anyone does anything in this community, people will throw faeces at people. Maybe we should do something about that?
User avatar
coiax
Joined: Sun Jan 31, 2016 10:45 am
Byond Username: Coiax

Re: We need a process for resolving PR's

Post by coiax » #173876

Anyway, this proposal is good, especially from the perspective of a casual/medium level coder.
User avatar
Hornygranny
Horny Police
Joined: Tue Apr 15, 2014 4:54 pm
Byond Username: Hornygranny

Re: We need a process for resolving PR's

Post by Hornygranny » #173882

Oldman Robustin wrote:People go after the author of the hated shit, not the mergerer, at least in OOC/Deadchat.
Not really true. "Coderbus" is flamed all the time for things getting merged, especially by those who don't bother to read github and look at the author. Go a couple years back and look at the ridiculous amounts of MUH INTERNAL DISCUSSIONS TOWER MUH CONSPIRACY posting when a lot of really controversial changes were being pushed.
Image
onleavedontatme
Joined: Fri Mar 13, 2015 10:26 pm
Byond Username: KorPhaeron

Re: We need a process for resolving PR's

Post by onleavedontatme » #173884

oranges wrote: What you called politicking and ingratiation is just built up trust, which you too will achieve in time.
Some of definitely is politicking. As you said, it was really shitty how Joan treated the cult PR.

On the other hand, if you continually go on about how "coders" are stupid/ruin the game/don't play, people are probably gonna be less enthusiastic to volunteer their time to help you get your code up to standard. That's gonna be the reality of things no matter whether your system is built to stop bias or not. Nobody is going to want to help someone who is yelling at them for a hobby.
Hornygranny wrote:The fundamental problem here is that we have a small group of maintainers who will ultimately bear some responsibility for what they merge. It can be a tedious job when there's no politics involved, but some vocal members of the community have repeatedly shown the maintainers that if they merge anything controversial they will get flamed endlessly. What this means is that for any big PR a maintainer essentially has to volunteer to be shit on by the community even though it wasn't their idea, and a lot of the time they're simply not willing to do it.
The flip side of this is people will yell at you forever if you close the PR as well, so it's easier to just ignore it them.
Oldman Robustin wrote: Kor I hope you recognize that you sit in the best position of the current system, I'd hoped you could better relate to how utterly shit the system is for others...
No I totally get the frustration. I spent a long time where you are now. I just don't think the things you've outlined here will realistically be implemented. It's still going to be a small handful of neckbeard volunteers merging 100+ bad ideas a week from other neckbeards.

I also think the system works a bit better than you credit it for considering your vision for cult has now twice overturned what two different maintainers were trying to do with it (and I was the one who helped you get the PR working despite not wanting old cult back at all!)
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

Re: We need a process for resolving PR's

Post by Oldman Robustin » #173903

Success with the cult PR was mostly made possible by the ~80% polling preferences for oldcult. That mandate was enough to pretty much ensure oldcult and my oldcult update would pull through.

But newcult was uniquely reviled, if only 55-65% of players preferred oldcult I can all but guarantee that the "improve newcult, forget oldcult" camp would've prevailed. I won't have that advantage going into future PR's, if it was so exhausting getting something with broad popular support done, why invest myself in another big project that won't have that kind of advantage?
Last edited by Oldman Robustin on Thu May 05, 2016 4:46 am, edited 1 time in total.
Image
lumipharon
TGMC Administrator
Joined: Mon Apr 28, 2014 4:40 am
Byond Username: Lumipharon

Re: We need a process for resolving PR's

Post by lumipharon » #173937

The amount of time we've had truly terrible prs get merged quickly, despite experienced and knowledgable players clearly and accurately pointing out their flaws and consequences are, is sadly hilarious.
User avatar
ShadowDimentio
Joined: Thu May 08, 2014 3:15 am
Byond Username: David273

Re: We need a process for resolving PR's

Post by ShadowDimentio » #173950

ShadowDimentio wrote:>Shit change is pushed through
>Community backlash
>Coders saying it's their fault for not complaining on git
>Someone eventually steps up and puts up a revert
>Revert is immediately closed because m-muh kneejerk, m-muh codebaby
>Community all yells what the fuck
>Revert is put up again
>Shit change is reverted after lengthy delay
>Go back to one after a week

THE CYCLE CONTINUES
BREAK THE CYCLE OLDMAN. YOU CAN BE THE ONE.
Spoiler:
"Clowns are different you can't trust those shifty fucks you never know what they're doing or if they're willing to eat a dayban for some cheap yuks."
-Not-Dorsidarf

"The amount of people is the amount of times the sound is played... on top of itself. And with sybil populations on the shuttle..."
-Remie Richards

"I just spent all fucking day playing fallen london and sunless sea and obsessing over how creepy the fucking dawn machine is and only just clocked now that your avatar is the fucking dawn machine. Nobody vote for this disgusting new sequence blasphemer he wants to kill the gods"
-Stickymayhem

"Drank a cocktail of orange Gatorade and mint mouthwash on accident. Pretty sure I'm going to die, I am on the verge of vomit. It was nice knowing you guys"
-PKPenguin321

"You're too late, you will have to fetch them from the top of my tower, built by zombies, slaves, zombie slaves and garitho's will to live!"
-Armhulen

"This is like being cooked alive in a microwave oven which utilises the autistic end of the light spectrum to cook you."
-DarkFNC

"Penguins are the second race to realise 2D>3D"
-Anonmare

"Paul Blart mall cops if they all had ambitions of joining the Waffen-SS"
-Anonmare

"These logs could kill a dragon much less a man"
-Armhulenn

">7 8 6
WHAT MADNESS IS THIS? POETIC ANARCHY!"
-Wyzack

"We didn't kick one goofball out only to have another one come in like a fucking revolving door"
-Kraseo

"There's a difference between fucking faggots and being a fucking faggot."
-Anonmare

"You guys splitting the 20 bucks cost to hire your ex again?"
-lntigracy

"Wew. Congrats. It's been actual years since anyone tried to make fun of me for being divorced. You caught me, I'm tilted. Here is your trophy."
-Timbrewolf

"I prefer my coffees to run dry too *snorts a line of maxwell house*"
-Super Aggro Crag

"You don't have an evil bone in your body, unless togopal comes for a sleepover"
-Bluespace

">Paying over a $1000 for a lump of silicon and plastic
Lol"
-Anonmare

"Then why did you get that boob job?"
-DrPillzRedux

"You take that back you colonial mongrel"
-Docprofsmith

"I don't care whether or not someone with an IQ 3 standard deviations below my own thinks they enjoy Wizard rounds."
-Malkraz
User avatar
oranges
Code Maintainer
Joined: Tue Apr 15, 2014 9:16 pm
Byond Username: Optimumtact
Github Username: optimumtact
Location: #CHATSHITGETBANGED

Re: We need a process for resolving PR's

Post by oranges » #173964

can he be the one to stop you posting?
User avatar
PKPenguin321
Site Admin
Joined: Tue Jul 01, 2014 7:02 pm
Byond Username: PKPenguin321
Github Username: PKPenguin321
Location: U S A, U S A, U S A

Re: We need a process for resolving PR's

Post by PKPenguin321 » #173980

>oldman makes controversial PR
>mixes in a new feature to compensate for it (but claim that "it's inter-related" when it's clearly not)
>kor doesn't fall for it and refuses to merge it until controversial part is removed
>oldman: "clearly it is the system that is flawed"

hahahaha

here's a solution for you:
1. atomize your PRs (don't give me the "inter-related" bullshit)
2. suck it up when your controversial shit gets denied. your code is not entitled to be in the game. it got denied. the only reason your pr is still open at all is because it's not atomized.
i play Lauser McMauligan. clown name is Cold-Ass Honkey
i have three other top secret characters as well.
tell the best admin how good he is
Spoiler:
Image
User avatar
Bawhoppennn
Github User
Joined: Wed Jan 14, 2015 11:42 pm
Byond Username: Bawhoppennn
Github Username: Bawhoppen

Re: We need a process for resolving PR's

Post by Bawhoppennn » #173988

PKP going in for the kill
I consider myself a /tg/station historian. If you're interested in the server history at all, feel free to ask me and I'll try and get you an answer! #ConcurForever

Image
<KorMobile> you're a hero

[21:20:53] <%oranges> Baw "has cute legs" hoppen
Image
DEAD: ADMIN(Owegno) says, "Nothing lewd happens in adminbus sadly."

[07:13:57] <Rockdtben> Keep in mind that I'm an extremely successful and wealthy male in his late twenties.

(F) DEAD: Professor DonkPocket says, "Admins preventchaos with good messages"

OOC: Pogoman122: Fun fact if someone trespasses on your kitchen just turn them into a nugget

Image

<+KorPhaeron> russians have no souls so magic enrages them
<+KorPhaeron> people who don't like rng are not from /tg/ and are likely redditors
ausops wrote:apart from this there is literally nothing more to say other than that this is the first thread in five years to have achieved something.
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

Re: We need a process for resolving PR's

Post by Oldman Robustin » #174064

PKP you dumbass this post is older than Kor's comment, the only reason Kor probably commented on it was because of my incessant whining in this thread.

Anyway, someday someone's going to have to sit down with me and explain why virtually every big balance change mixes features with tweaks, but at some point (usually based on what a certain individual feels about a change) people will argue that you have to break them up.

1) I tried to get rid of the ling snowflake rules for holoparasites after witnessing several traitors ling get crates and then yell at me in ahelp for screwing them. I honestly believe that holoparasites and lings are not as synergistic as people believe. Losing revive is a BIG deal.

2) That PR fails and the most common comment is "remove traitorling instead!"

3) I'm ok with traitorlings but I'm sick of traitors AND lings being balanced around this snowflake antag, so I contemplate removing traitorling and the snowflake restrictions at the same time as a COMPROMISE

4) However, I'm still un-easy with the absolute separation of lings and traitors during traitorling. Lings buying syndiekeys was an entertaining meme that led to some interesting rounds. To address this issue I had the idea of adding HIVEMIND LINK that would let lings potentially extract useful information, items, or alliances from crew... most importantly traitors who could offer their TC in exchange for their lives. Obviously the ling has a superior negotiating position so it would be up to them whether to honor the bargain (if I verified they were traitors I'd prefer a potential ally to a corpse).

5) Ok, I've reached a middle-ground that addresses all the big concerns, surely such a compromise belongs in the same PR? PKP/Kor: lol no, I only like SOME of these features so please remove the ones I like so I can merge them and then make sure the stuff I don't like sits in purgatory forever.

I mean I explicitly and honestly assembled that PR as a compromise package for a contentious issue that has yet to be resolved. Why the hell are some of you so eager to tear that apart into some piecemeal changes even -I- as the damn author wouldn't support individually. Every day there are PR's that mix features with tweaks, nobody complains when Joan adds 5 new blob chemicals but also drastically changes the power of blobbernauts. Shouldn't the standard always just be "If they're not related, atomize them" not "I don't like some of these changes, atomize them"?

Anyway I've asked Kor to take a second look since me and Coiax JUST nerfed fleshmend and Kor overlooked stuff like Holoparasites doing clone damage in crit so a ling+holoparasite taking a heavy beating is basically on a death sentence unless they go get cryo'd. And, of course the most important point for last, that PR is not what prompted this post. I only used it to point out that it took 12 days for Kor even tell me what the holdup was. The PR has majority support and is a compromise position on a contentious issue. People are so split that Kor managed to get the holoparasite exception in but got cockblocked on the sleeping carp snowflake restriction, further emphasizing the need for a sustainable solution for traitorling rounds... coming up with a decent compromise that MOST people supported is no small miracle.
Image
User avatar
Cheridan
Joined: Tue Apr 15, 2014 6:04 am
Byond Username: Cheridan

Re: We need a process for resolving PR's

Post by Cheridan » #174082

Oldman Robustin wrote:Shouldn't the standard always just be "If they're not related, atomize them" not "I don't like some of these changes, atomize them"?
...That's exactly the reason why we ask that PRs be atomic, though. If your PR has 5 changes and they're all good and everyone likes them then nobody gives a shit, go on and merge it. If your PR has 5 changes and 1 of them is contentious then, uh, yeah, it's going to hold up the other 4 while people argue about it.
Image
/tg/station spriter, admin, and headcoder. Feel free to contact me via PM with questions, concerns, or requests.
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

Re: We need a process for resolving PR's

Post by Oldman Robustin » #174207

Cheridan wrote:
Oldman Robustin wrote:Shouldn't the standard always just be "If they're not related, atomize them" not "I don't like some of these changes, atomize them"?
...That's exactly the reason why we ask that PRs be atomic, though. If your PR has 5 changes and they're all good and everyone likes them then nobody gives a shit, go on and merge it. If your PR has 5 changes and 1 of them is contentious then, uh, yeah, it's going to hold up the other 4 while people argue about it.
Well the way I see it is my 3 changes are interrelated and we've had the debate in the comments, opening another to rehash everything would serve nobody. Just hoping Kor changes his mind, fleshmend JUST got slapped with a nerf that me and Coiax worked on, that and the clone damage and loss of revive and no traitorling thing makes ling holoparasites a non-issue. Since it's the entire reason I made the PR, to get rid of snowflake rules for lings, would be disappointed if we still refused to end the ling-exception.
Image
User avatar
PKPenguin321
Site Admin
Joined: Tue Jul 01, 2014 7:02 pm
Byond Username: PKPenguin321
Github Username: PKPenguin321
Location: U S A, U S A, U S A

Re: We need a process for resolving PR's

Post by PKPenguin321 » #174221

Only they're not interrelated, each individual change could stand on its own. You're just being stubborn about your ling+holo meme. Get over yourself
i play Lauser McMauligan. clown name is Cold-Ass Honkey
i have three other top secret characters as well.
tell the best admin how good he is
Spoiler:
Image
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

Re: We need a process for resolving PR's

Post by Oldman Robustin » #174231

PKPenguin321 wrote:Only they're not interrelated, each individual change could stand on its own. You're just being stubborn about your ling+holo meme. Get over yourself
If it's the very reason that I spent hours trying to get humans to talk in Lingchat, I don't think its "unrelated".

Virtually every tweak + feature PR could break up the balance stuff to "stand on its own".

In my Cult #2 PR I change the values on Unholy water to heal cultists. I also add a feature where you can use an altar to summon unholy water. Each change can easily stand on its own, but you'd be pretty retarded to argue that I should break them up into separate PR's. What's the difference here?
Image
User avatar
PKPenguin321
Site Admin
Joined: Tue Jul 01, 2014 7:02 pm
Byond Username: PKPenguin321
Github Username: PKPenguin321
Location: U S A, U S A, U S A

Re: We need a process for resolving PR's

Post by PKPenguin321 » #174240

Oldman Robustin wrote:
PKPenguin321 wrote:Only they're not interrelated, each individual change could stand on its own. You're just being stubborn about your ling+holo meme. Get over yourself
If it's the very reason that I spent hours trying to get humans to talk in Lingchat, I don't think its "unrelated".
I'm not saying it's unrelated, but I'm also not saying that it can't be merged on it's own and still be a valuable feature without all the controversial bullshit tacked on.
i play Lauser McMauligan. clown name is Cold-Ass Honkey
i have three other top secret characters as well.
tell the best admin how good he is
Spoiler:
Image
onleavedontatme
Joined: Fri Mar 13, 2015 10:26 pm
Byond Username: KorPhaeron

Re: We need a process for resolving PR's

Post by onleavedontatme » #174272

Oldman Robustin wrote:PKP you dumbass this post is older than Kor's comment, the only reason Kor probably commented on it was because of my incessant whining in this thread.
https://github.com/tgstation/-tg-station/pull/16497

Except I commented on that one back in march

https://github.com/tgstation/-tg-station/pull/16457

And this one back in march

And buggy's as well but I can't find that PR and searching is a pain on mobile.

So yes I ignored the PR the 4th time it was made.

>why are dumb overpowered things being waved through where is the quality control someone needs to put their foot down
>also my antag buff isnt being merged fast enough its not fair for someone to say no
>also its not fair antags keep getting buffed security is suffering
>fuck all coders

Gonna get yelled at no matter what. Deny or approve, almost any PR will be taken as a personal affront by some number of people (or you can compromise and make them all mad at once!)

EDIT: I really am sorry the cult thing happened the way it did though. Was garbage. Trying to code for /tg/ is a constant battle of wills and I know it's exhausting. If you can't take "no" for an answer though then you're part of why we design by shouting instead of a any sort of leader.
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

Re: We need a process for resolving PR's

Post by Oldman Robustin » #174277

Your comments, while informative, still don't address the issue where they sit around for weeks unattended:

" KorPhaeron commented 26 days ago

I previously tried to do this and everyone called the PR bad, while Incoming cited antag mixups as his primary motivation for datum antags. I more or less agree with it, but I think it will make other people unhappy."

This is better than nothing, but it's not the kind of comment that let's me make progress on my PR. More people liked it than disliked it, which couldn't be said about past efforts by others to tackle this issue, I felt my compromise was a good solution for a persistent problem.

Also, I had to re-open the PR after I deleted my repo :^), it had nothing to do with trying to re-market it. I didn't even want to make a new one but Raz insisted that I make some minor code changes.

It was re-opened with the assurance that at 25 days old, reopening it would mean I'd have to wait another month to get it resolved. I'm prepared to compromise on the compromise PR that still had majority support, but I want to make sure you're taking into account all the factors Coiax and I made in our recent comments. I'm not joking when I said that removing the snowflake rules was the impetus behind it. I got ahelped 2 rounds in a row from lings who couldn't use HP's and I was sympathized with the BS situation they were forced into, that was literally was sparked all this, so be told "lol its not related, atomize it" is frustrating.
Last edited by Oldman Robustin on Fri May 06, 2016 3:16 pm, edited 2 times in total.
Image
User avatar
PKPenguin321
Site Admin
Joined: Tue Jul 01, 2014 7:02 pm
Byond Username: PKPenguin321
Github Username: PKPenguin321
Location: U S A, U S A, U S A

Re: We need a process for resolving PR's

Post by PKPenguin321 » #174290

a maintainer (and not just any maintainer, but the design lead), has disapproved of your PR

either change it according to what he said (atomize) or your PR will rot until it gets closed

this is how things work. the system is not flawed, your PR is

that is all
i play Lauser McMauligan. clown name is Cold-Ass Honkey
i have three other top secret characters as well.
tell the best admin how good he is
Spoiler:
Image
User avatar
Wyzack
Joined: Fri Apr 18, 2014 11:32 pm
Byond Username: Wyzack

Re: We need a process for resolving PR's

Post by Wyzack » #174293

Kor u cuck your selected course of action was shit
Arthur Thomson says, "Since there are no admins I would loging with another account and kill you"
Caleb Robinson laughs.
Arthur Thomson catches fire!
tusterman11 wrote:Can you stop lying? I just asked you and you are was a piece of shiit on me!!!
Kor wrote:I wish Wyzack was still an admin.
EngamerAzari's real number one fangirl <3
certified good poster
User avatar
Oldman Robustin
Joined: Tue May 13, 2014 2:18 pm
Byond Username: ForcefulCJS

Re: We need a process for resolving PR's

Post by Oldman Robustin » #174365

PKPenguin321 wrote:a maintainer (and not just any maintainer, but the design lead), has disapproved of your PR

either change it according to what he said (atomize) or your PR will rot until it gets closed

this is how things work. the system is not flawed, your PR is

that is all
>>>>>>>>Speaking on behalf of Kor meme
>>>>>>>>Still acting like this is about a single PR meme
>>>>>>>>Implying I'm not ready to meet the requested conditions but wanted to make sure all information was considered maymay
>>>>>>>>STILL implying that this thread came after Kor's response, when it actually preceeded it MEEMEEE

The Hivemind Link could get closed tomorrow with 100x middle finger emotes as the explanation and I wouldn't have made a post about that PR. I made this post because my willpower to keep working on the code is being sapped by the time, energy, and mental distraction caused by the fact I can't get any progress on any PR that isn't a grammar/flavor change without harassing people in IRC, and even with harassment some PR's just don't have a way forward until someone sacks up and just says "heres what you need to do". The proposed system should be an improvement for ALL parties and the biggest complaint I've heard is that it won't fix all of our problems.
Image
Lati
Joined: Mon Oct 12, 2015 4:40 pm
Byond Username: Lati

Re: We need a process for resolving PR's

Post by Lati » #174403

I wish there was some way of people to self-add "ready for review" tags on their PRs. That would mean that it's completely ready and maintainers can merge them after checking that everything looks okay. This wouldn't solve the problem for controversial things (which are the slowest and most problematic) but would still speed up the overall process when maintainers can choose these PRs starting from the oldest ones and check/deny/merge them. I've had some of my PRs get stuck for over a week even with non-controversial small fixes or features that got merged in the end after some nice maintainer managed to check the code.
User avatar
duncathan
Joined: Mon May 25, 2015 4:12 pm
Byond Username: Dunc
Github Username: duncathan

Re: We need a process for resolving PR's

Post by duncathan » #174576

I really wish I had time to review more than one PR a week right now.
Image
Players can and will create their own fun.
User avatar
MisterPerson
Board Moderator
Joined: Tue Apr 15, 2014 4:26 pm
Byond Username: MisterPerson

Re: We need a process for resolving PR's

Post by MisterPerson » #174646

Lati wrote:I wish there was some way of people to self-add "ready for review" tags on their PRs. That would mean that it's completely ready and maintainers can merge them after checking that everything looks okay. This wouldn't solve the problem for controversial things (which are the slowest and most problematic) but would still speed up the overall process when maintainers can choose these PRs starting from the oldest ones and check/deny/merge them. I've had some of my PRs get stuck for over a week even with non-controversial small fixes or features that got merged in the end after some nice maintainer managed to check the code.
While useful for a variety of tags like DNM, Merge Conflict, and others, frankly if a PR isn't in need of review or even able to be reviewed, I have to question why you even have it open in the first place. You can always reopen it later if necessary.

Too bad Github development is slow, random, and sporadic. Expect to see it in 2021 the week after we migrate to Gitlab.

Edit: Oh you meant like "Ready for merge" hurp derp I'm dumb. Yeah that would be swell.
I code for the code project and moderate the code sections of the forums.

Feedback is dumb and it doesn't matter
User avatar
mercenaryblue
Joined: Fri Jul 24, 2015 12:26 am
Byond Username: Mercenaryblue

Re: We need a process for resolving PR's

Post by mercenaryblue » #175013

We do need a process. We need a council.

New PR comes up and 24h pass, the maintainer council gets to vote. Majority wins.
Then the council gives the PR a review on what to fix, if needed, and one week to fix it.
Unless the PR was extremely popular and ready for a merge, then they can just merge it after the vote.
User avatar
oranges
Code Maintainer
Joined: Tue Apr 15, 2014 9:16 pm
Byond Username: Optimumtact
Github Username: optimumtact
Location: #CHATSHITGETBANGED

Re: We need a process for resolving PR's

Post by oranges » #175084

John Oxford is that you?
callanrockslol
Joined: Thu Apr 24, 2014 1:47 pm
Byond Username: Callanrockslol

Re: We need a process for resolving PR's

Post by callanrockslol » #182401

We should just make me headcoder and all will be fine.
The most excessive signature on /tg/station13.

Still not even at the limit after 8 fucking years.
Spoiler:
Urist Boatmurdered [Security] asks, "Why does Zol have a captain-level ID?"
Zol Interbottom [Security] says, "because"

Sergie Borris lives on in our hearts

Zaros (No id) [145.9] says, "WITH MY SUPER WIZARD POWERS I CAN TELL CALLAN IS MAD."
Anderson Conagher wrote:Callan is sense.
Errorage wrote:When I see the win vista, win 7 and win 8 hourglass cursor, it makes me happy
Cause it's a circle spinning around
I smile and make circular motions with my finger to imiatate it
petethegoat wrote:slap a comment on it and call it a feature
MisterPerson wrote:>playing
Do you think this is a game?
Gun Hog wrote:Untested code baby
oranges wrote:for some reason all our hosts turn into bohemia software communities after they implode
Malkevin wrote:I was the only one that voted for you Callan.
Miggles wrote:>centration development
>trucking
ill believe it when snakes grow arms and strangle me with them

OOC: Aranclanos: that sounds like ooc in ooc related to ic to be ooc and confuse the ic
OOC: Dionysus24779: We're nearing a deep philosophical extistential level

Admin PM from-Jordie0608: 33-Jan-2552| Warned: Is a giraffe dork ~tony abbott

OOC: Saegrimr: That wasn't a call to pray right now callan jesus christ you're fast.

OOC: Eaglendia: Glad I got to see the rise, fall, rise, and fall of Zol

OOC: Armhulenn: CALLAN
OOC: Armhulenn: YOU MELTED MY FUCKING REVOLVER
OOC: Armhulenn: AND THEN
OOC: Armhulenn: GAVE ME MELTING MELONS
OOC: Armhulenn: GOD FUCKING BLESS YOU
OOC: Armhulenn: you know what's hilarious though
OOC: Armhulenn: I melted ANOTHER TRAITOR'S REVOLVER AFTER THAT

7/8/2016 never forget
Armhulen wrote:
John_Oxford wrote:>implying im not always right
all we're saying is that you're not crag son
bandit wrote:we already have a punishment for using our code for your game, it's called using our code for your game
The evil holoparasite user I can't believe its not DIO and his holoparasite I can't believe its not Skub have been defeated by the Spacedust Crusaders, but what has been taken from the station can never be returned.

OOC: TheGel: Literally a guy in a suit with a shuttle full of xenos. That's a doozy
User avatar
paprika
Rarely plays
Joined: Fri Apr 18, 2014 10:20 pm
Byond Username: Paprka
Location: in down bad

Re: We need a process for resolving PR's

Post by paprika » #182428

While I understand Robustin's frustrations and see why his solutions could be good under certain circumstances, I believe a more platitudes-based system for Pull Request review would benefit the codebase cohesion better.
Oldman Robustin wrote:It's an established meme that coders don't play this game.
Post Reply

Who is online

Users browsing this forum: No registered users