While I really understand QA, what I do not understand is ::
1. Why there are so many restrictions on PRs?? I also think that maintaining good quality is - somehow - key to success, but, be fair, this is too strict. In my opinion, there should be 2 (and only 2) cases of PR's wthdrawal::
a) PR being obvious crap (like blank file),
b) work-in-progress.
Rest should be accepted straight-away.
2. If you (=devs) get more signals that codebase is low quality, why dont you suspend release cycle,do a full rewrite and restore release cycle when rewrite is done? You will say that this would put MyBB into grave - no it would not. Doing full rewrite doesnt mean abandoning project.
1. PRs aren't accepted until they've been tested by a developer. Blindly merging PRs without testing them at all is a recipe for disaster. We also get some PRs wanting to add features that haven't been discussed or anything except by the PR author - we like to have a set direction that the software is heading in rather than adding or removing things at random.
2. 2.0 is a full rewrite and is work in progress. We're trying to balance time between 1.8 and 2.;0 as otherwise people would see the project as having stalled. Progress is slow enough as is without completely suspending development on the only functioning version of the codebase.
(2017-09-15, 01:26 PM)Euan T Wrote: [ -> ]1. PRs aren't accepted until they've been tested by a developer. Blindly merging PRs without testing them at all is a recipe for disaster. We also get some PRs wanting to add features that haven't been discussed or anything except by the PR author - we like to have a set direction that the software is heading in rather than adding or removing things at random.
2. 2.0 is a full rewrite and is work in progress. We're trying to balance time between 1.8 and 2.;0 as otherwise people would see the project as having stalled. Progress is slow enough as is without completely suspending development on the only functioning version of the codebase.
1. Not random; all PRs but in the order they appear.... btw you did not read my post carefully.
2. n/c; see p1.
I did read your post carefully.
We don't merge PRs in the order that they appear because larger PRs obviously take more time to test and review. PRs changing a spelling or grammatical error in a language file (for example) are extremely easy to review and can be merged after simply looking them over. Ones that add new features or substantially modify existing features obviously have a much greater chance of causing problems or introducing unintended side effects or security issues and as such take a far longer time to review.
We also tend to test PRs across a variety of different environments (such as on PHP 5, PHP 7.1 and now PHP 7.2 - 7.2 recently introduced some breaking changes in our existing code that we are now testing new code against).
Some other PRs (like the
currently open PR to fix an issue with SMTP DIGEST-MD5 or CRAM-MD5) need particular set ups to be tested against (such as, in the case mentioned before, a certain SMTP server setup). In that case, we cannot merge the PR until it has been tested against the required setup.
There's also the fact that everybody working on the MyBB project is a volunteer who doesn't get paid or compensated in any way for the time they spend on the project. Most of us either work 5 days a week or are at university or college rather than having a serious amount of time to devote fully to working on MyBB.
(2017-09-15, 01:48 PM)Euan T Wrote: [ -> ]I did read your post carefully.
We don't merge PRs in the order that they appear because larger PRs obviously take more time to test and review. PRs changing a spelling or grammatical error in a language file (for example) are extremely easy to review and can be merged after simply looking them over. Ones that add new features or substantially modify existing features obviously have a much greater chance of causing problems or introducing unintended side effects or security issues and as such take a far longer time to review.
We also tend to test PRs across a variety of different environments (such as on PHP 5, PHP 7.1 and now PHP 7.2 - 7.2 recently introduced some breaking changes in our existing code that we are now testing new code against).
Some other PRs (like the currently open PR to fix an issue with SMTP DIGEST-MD5 or CRAM-MD5) need particular set ups to be tested against (such as, in the case mentioned before, a certain SMTP server setup). In that case, we cannot merge the PR until it has been tested against the required setup.
There's also the fact that everybody working on the MyBB project is a volunteer who doesn't get paid or compensated in any way for the time they spend on the project. Most of us either work 5 days a week or are at university or college rather than having a serious amount of time to devote fully to working on MyBB.
I understand your point of view, but I
disagree with it. Wouldnt it be easier to let commiter test the code and assert the code has already been tested? Wouldnt it speed the merge proccess up?
Larger PRs takes more time? Once again - it depends on how fluent the reviewer is in PHP.
Once again - we have different understanding of how QA should look like;
Im strongly against current dev team. Elections?
Or revision of QA and workflow?
Or open repo (=make PR automergeable) for, say, 2 weeks. What do you say?
Well if we depended entirely upon users testing their code, today we would have merged a piece of code that used a variable without using a $ sign in front of it, as a small example. Had that been merged automatically with no oversight, how long would it have taken to notice that the upgrade script for 1.8.13 wasn't quite working correctly?
And no, being "fluent in PHP" doesn't determine how long it takes to review a PR. You can be Rasmus Lerdorf (who obviously is extremely "fluent in PHP") but you might not know exactly how functions within the MyBB co-operate and function with each other. The way that 1.8 is written makes heavy use of global variables and is extremely tightly coupled. A change (no matter how small it may seem on paper) could be altering a global variable depended upon in some template not touched in the PR itself or within some function elsewhere in the code in such a way that things break.
We are always accepting applicants to join the team, but one of the prerequisites is prior contributions to the project - if you're applying to join the dev or QA team, we expect to have seen either PRs or PR reviews on GitHub or several quality plugins or several vulnerability reports, etc.
The QA workflow at the minute is extremely simple. If you have a concrete suggestion for how we should be doing QA, we will at least read through it and if we agree, may implement at least parts of it.
GitHub (as far as I'm aware) has no way to make PRs automatically merge. And even if it did, my first paragraph still applies.
@Euan:
If GH does not allow automerge (I seriously think it does allow, but this option is buried somewhere deep), than move to BitBucket (it allows automerge for sure).
Wouldnt be hard to do as BB has smart import feature. Would be less tham 1 minute.
Just because it is easy to do doesn't mean it is a good idea...
(2017-09-16, 11:18 AM)Tom K. Wrote: [ -> ]Just because it is easy to do doesn't mean it is a good idea...
why do you say its not good idea?