Retroarch: Workflow issues with this project

[fr500]

I have several problems with the workflow on this project.

I'm a rather noob programmer, when I send a pull request I often ask for a review, case in point here: https://github.com/libretro/RetroArch/pull/6389

I asked a review because I suspected it could break some achievement related functionality.

I don't see why do we have to rush it and merge the PR without any form of review. Furthermore, if you really HAD TO FIX the PS3 updater, you could have just pulled the URLs directly (and fixing the updater and releasing a stable seems rather pointless considering the fact that using the updater on console platforms makes the builds no longer stable since RA is static linked to the cores).

I guess this isn't a big one, but as a contributor is PRETTY DAMN ANNOYING to have to rush a fix now because the button had to be pressed ASAP!

The same happens with major changes / rewrites, they happen on master branch without any consideration for breakage which often means massive commit rollbacks at the last minute and ends up on some contributors having to redo their pull requests.

I don't know the solution to this problem, but I do think some changes need to happen. This is pretty discouraging for me in particular.

[twinaphex]

If you had split up the PR into two and made the PS3 changes distinct from the Cheevos changes, this could have been avoided.

I would be happy to merge a PR undoing the cheevos changes if they truly cause issues and then make another Pr for them again, which this time will not get merged until leiradel reviews it.

In any case, honestly, in my opinion the workflow issues stem elsewhere - cheevos should not be doing its own URL encoding, it should use the same URL encoding as all other code in RetroArch does so that we dont run into code rewrite roadblocks like this when changing something. We need to make the cheevos code maintainable eventually by more than one person, otherwise issues like this will continue to happen.

[twinaphex]

That being said, I could be less trigger happy with PRs, yes. I will be more considerate when it comes to code reviews.

[fr500]

IF someone asks for a review, the only sensible thing to do is to wait for
such a review.
The only reason I even sent a PR is because you were in such a hurry for
PS3 updater to work but you weren't around on chat, I actually explained
the issue and you could have just fixed the URLs instead.

He actually replied to me via PM since the PR was closed already when he
found it. He explained his reasoning to me, the URL encode we have wouldn't
cut it actually so this needs to happen elsewhere. But that's just
specifics that do not matter in the argument.

The argument is that everyone's reaction this morning was... "UGH, why was
this merged?"

We're all on different time zones, it may seem a lot to you but waiting at
least a day for a response should be a policy or something.

And the other problem, big re-writes on master... sorry but that flat out
sucks, you're fully aware of what's happened with the hash removal changes
on the last two occasions, nothing untested should go into master in my
opinion. Of course there is the other problem, there aren't enough testers,
and no builds of branches, but breaking, reverting, fixing, force pushing
should be avoided.

On Thu, Mar 15, 2018 at 11:01 AM, Twinaphex [e-mail]
wrote:

That being said, I could be less trigger happy with PRs, yes.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/libretro/RetroArch/issues/6394#issuecomment-373428801,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABpC0Ml6ggy1ZqxZF8a5i2ENxhSmMQbMks5tepBigaJpZM4SsaZF
.

[twinaphex]

Alright, I am sorry. Next time a PR asks for a review, I will wait for that review and doublecheck before merging anything.

[fr500]

Thanks, that's cool.