Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: How to have PRs merged faster without lowering quality  (Read 22081 times)

0 Members and 1 Guest are viewing this topic.

Ceylo

  • Hero Member
  • *****
  • Posts: 2325
    • View Profile
    • http://sfemovie.yalir.org/
    • Email
How to have PRs merged faster without lowering quality
« on: March 28, 2018, 12:14:19 am »
Hello,

I'm opening this topic because I feel like current PR submission process is not efficient and I believe this is discouraging people from contributing more (at least it discourages me).

To give a few numbers:
PRTesting & feedbacksWaiting to be merged
#13352 months2 days
#137112 days7 days
#13612 days10 days
#13343 weeks7 days
#1377~10 days*7 days*
#13952 days4 days
#13844 days3 days
#13562 weeks1 day
#13502 weeks0 day
#13452 days7 days
#13427 days7 days
* there was no feedback so not easy to tell if any testing phase happened and for how long

I hope this is representative enough. Of course I made this table mainly because I'm somewhat traumatized with the 2 months testing phase on my PR, that was mainly waiting and no feedback. But it's not the only PR in this case (at a smaller scale though).

There are several cases:
1. there are feedbacks and changes are required -> to me this is not an issue, so let's not focus on it
2. there are no feedbacks
  a. on (code) review -> should be improved
  b. on testing -> should be improved
3. PR is ready to be merged (code review, testing, feedbacks and CI are ok) but still waits to be merged -> should be improved

Of course I understand that the SFML core team can't do everything right away, and this is not what I wish. So here are a few ideas to help on these matters.


2.a : I don't know what are the current rules. Should the (code) review be done always by at least a SFML core team member, or can it be done by any member? In both cases my personal feeling is that currently the PRs that are blocked by a (code) review are not visible in SFML's PR list. The tags in https://github.com/SFML/SFML/pulls don't look to give this information.

To "fix" this we would need an easy way to see the list of PRs that are waiting for review. I can see 2 ways of doing that: a new tag on which you could filter on, on SFML's GitHub, or a forum section with a topic per PR needing review. This last solution looks maybe overkill but the advantage is that it's visible from all SFML forum members that would visit this section, and if people want to help on SFML development by reviewing, they can know where to help in an easy way. If we go this way it should be the PR's author's responsibility to open the topic in the forum section, so that it's not more burden for SFML core team.

2.b : Same questions about current requirements on testing. From what I saw it depends on whether the change looks "safe" or not. If it's safe testing is skipped, apart from CI. When testing is needed, can it be done by anyone or should there be at least 1 tester from SFML core team? Again this is very similar to 2.a. And we could most likely apply the same solution.

3. I noticed that only eXpl0it3r is merging the PRs. Why? I would expect that at least all members from the SFML core team could do this. Also I wonder if there is currently a minimal wait time before merge (I remember there were notices like "If nobody is against this PR withing X days it will get merged" in the past).

The goal would be for any PR that don't need updates (ie. only review and testing time) to be merged within at most 2 days. This is of course not doable if parts of the process can be done by too few people, but totally doable if the process is scalable.

Ceylo
Want to play movies in your SFML application? Check out sfeMovie!

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4321
    • View Profile
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #1 on: March 28, 2018, 08:18:16 am »
Here is my opinion on the matter. If others team members disagree they will say it.

First of all, thanks for your ideas. I really appreciate community involvement in such matters. :)

We wish for more code review from the community!

Having more community feedback on code quality helps triage good and bad PRs. With more feedback we can also have a better appreciation of the need for new features and when it comes to bugs, we cannot reproduce everything and therefore NEED community reviews, in addition to testing.

Ultimately my feelings is that the whole team wants to ensure SFML keeps a high code quality standard. It's part of its 'personality'. That's why I suggest we request the final approval by a team member. My hope is that with more feedback this will go much faster as grey area in the code would already have been discussed.

Your suggestion for adding a tag and putting more light on those blocked PR is a good one, I think.

I've tried promoting a few of my PRs on the forun a while back. Sadly, it didn't work. It also adds some work. Maybe we should instead regularly advertise that people can subscribe to github issues if they want to contribute. But in any case, people can always open a thread for a specific PR and attract the attention of the community.

Regarding testing, community feedback is absolutely desirable, even required for most bugfix. When we have several people regularly accurately testing features/bugfixes we will be able to rely on them and therefore reduce the workload of the team member (so that we can focus on code review, merging stuff, design discussions, maintaining servers, ci and more).

It's true that we have relied solely on eXpl0it3r for the merging procedure, and that for quite a while. He's doing a fantastic job for sure. But maybe, eXpl0it3r, do you want other teamates to help? That goes without saying that other members should find some time for this as well.

Currently merging is done in batches to make rebating less tedious. If we merge a PR everyday or so, every other PRs need to be updated. So we have to find a tradeoff.
SFML / OS X developer

aggsol

  • Newbie
  • *
  • Posts: 24
  • C++
    • View Profile
    • My Github Account
Re: How to have PRs merged faster without lowering quality
« Reply #2 on: March 28, 2018, 08:50:49 am »
From my outside view visibilty is a problem and the process is not so clear at all. There are SO MANY issues in the tracker! I also find anything under 3 weeks is acceptable.

EDIT: I just had a look at the issues, some are from 2011. Even though those are feature requests those should be implented by now or been closed as nobody seems to need them in the past 7 years. There is something wrong with the process or SFML ist just not attractive.
« Last Edit: March 28, 2018, 08:55:06 am by aggsol »

Laurent

  • Administrator
  • Hero Member
  • *****
  • Posts: 32504
    • View Profile
    • SFML's website
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #3 on: March 28, 2018, 10:31:33 am »
Here are some random thoughts about this subject:

1. eXpl0it3r is amazing at managing tasks, I wouldn't say he is the bottleneck, and I think having more people involved in this process would just make it a big mess -- unless eXpl0it3r is fed up and wants some help, of course :)

2. Reviewing and testing needs time, I don't think there's any good reason for rushing to merge a PR as long as  it is still active. The problem, in my opinion, is when PRs get stuck (or worse: forgotten) because there's suddenly no one to test or review it.

3. We've already tried various strategies for getting more people involved in the review & testing process, but they all failed. The truth is that only the author of the PR and, with some luck, one or two SFML team member, really cares to actively work on PRs. Others just wait until it's done and available.

4. Unlike you, I think we're doing a really great job at handling issues and PRs. Issues and PRs are submitted quite frequently, and so far we've been able to handle them, allowing SFML to remain active and evolve even without the team members working on it. Compare the situation to what it was before: I think we've made huge progress.

So the only problem is the lack of testing & review from the community, and in my opinion this cannot be solved. People who want to help already do it. There's nothing we can do to force others to get involved.
Laurent Gomila - SFML developer

Ceylo

  • Hero Member
  • *****
  • Posts: 2325
    • View Profile
    • http://sfemovie.yalir.org/
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #4 on: March 28, 2018, 11:14:40 pm »
Ultimately my feelings is that the whole team wants to ensure SFML keeps a high code quality standard. It's part of its 'personality'. That's why I suggest we request the final approval by a team member. My hope is that with more feedback this will go much faster as grey area in the code would already have been discussed.
Agreed.

Your suggestion for adding a tag and putting more light on those blocked PR is a good one, I think.
I really insist about the fact that the PR creator must be able to set the tags himself (if we go for the tags solution). It’s important that there is no need to wait for anyone from the core team to set the tag, and that we don’t wait for the PR to look blocked : the tag should be set as soon as the PR can be reviewed.

I've tried promoting a few of my PRs on the forun a while back. Sadly, it didn't work. It also adds some work.
Was it mixed with other topics or in a dedicated section? To me it’s really important that people who help by reviewing or testing can do it without searching for these topics. There should be list that shows where help is wanted. Topics in a distinct section is one possible way to expose this list.

As for the additional work, if you consider that the PR's author does it, it's not more work for the core team, and only a little bit more for each contributor.

Maybe we should instead regularly advertise that people can subscribe to github issues if they want to contribute. But in any case, people can always open a thread for a specific PR and attract the attention of the community.
I'm afraid we won't get results if we can't make this become a habit, I mean the people who'd like to help would maybe not get used to help if a topic requesting help is created only sometimes. You could think of this as virtually creating an easy workflow for people who want to help.

Currently merging is done in batches to make rebating less tedious. If we merge a PR everyday or so, every other PRs need to be updated. So we have to find a tradeoff.
"rebating"?
Why do other PRs need to be update after a merge? Because you're not confident that once merged, even if there is no conflict it could break builds? I can't remember how it's called but the CI could check 2 things, the current branch state (what is currently done), and create a temporary merge commit (by merging from master into the PR's branch) and provide build results for this situation too. This way you remain 100% sure the PR can be merged without breaking master, and the PR authors don't need to update their PR. Then you don't need PR batching. This is more load for your builders but it seems like it could handle it. More computer time and less human time.

From my outside view visibilty is a problem and the process is not so clear at all. There are SO MANY issues in the tracker!
Not willing to upset you but the PR process is already big enough to deal with, so let's remain focused :) If you want to talk about GitHub issues you should open a distinct thread.

1. eXpl0it3r is amazing at managing tasks, I wouldn't say he is the bottleneck, and I think having more people involved in this process would just make it a big mess -- unless eXpl0it3r is fed up and wants some help, of course :)
I agree eXpl0it3r is doing an amazing job. But if PRs wait a full week to be merged there's still something wrong. For now I still don't know if the delay if because of batching or because the free time of a single person is not enough so will wait for eXpl0it3r's answer on this matter.

2. Reviewing and testing needs time, I don't think there's any good reason for rushing to merge a PR as long as  it is still active. The problem, in my opinion, is when PRs get stuck (or worse: forgotten) because there's suddenly no one to test or review it.
Nobody talked of rushing :) only being more efficient to reduce the time between when the PR is opened and when it is merged. Which most likely implies sharing more work with the community.

3. We've already tried various strategies for getting more people involved in the review & testing process, but they all failed. The truth is that only the author of the PR and, with some luck, one or two SFML team member, really cares to actively work on PRs. Others just wait until it's done and available.
Can you give more details about what you tried?

4. Unlike you, I think we're doing a really great job at handling issues and PRs. Issues and PRs are submitted quite frequently, and so far we've been able to handle them, allowing SFML to remain active and evolve even without the team members working on it. Compare the situation to what it was before: I think we've made huge progress.
I'm not saying you're not doing a great job. You do a great work with current team. But that doesn't mean it (the process, not your work ;D ) is satisfying from PR authors point of view, because of the delays mentioned above. I don't know how the situation previously was so I believe you about the progress.
But let's take my last PR as an example (although it's an extreme case) for which testing took 2 months, where probably 90% of that time is waiting for feedback. During that time I refrained from helping on other matters, because I didn't want to pile unmerged work up, I wasn't sure it would be merged at all, and it was rather demotivating to get sparse feedback. So what you seem to consider a good enough process means that I could have contributed more or less 3 times more to SFML in the same time range. Up to you to consider if it's worth it.

So the only problem is the lack of testing & review from the community, and in my opinion this cannot be solved. People who want to help already do it. There's nothing we can do to force others to get involved.
Waiting for your answer on 3. before answering this. But yes there are things to do, and you can't force people to get involved, but you can motivate them.
Want to play movies in your SFML application? Check out sfeMovie!

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4321
    • View Profile
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #5 on: March 29, 2018, 07:51:52 am »
Regarding the promotion posts: they were in a dedicated thread, maybe it was pinned. I don't remember. But it wasn't in a dedicated sub forum section. That, we haven't tried.

I'm afraid that in the end the team would have to clean up / maintain a new subsection dedicated to that. But we haven't actually tried... So why not? In the worst case we close the subforum.

I think we can make the overall process a bit more accessible to contributor through github templates for issues and pull requests. We already have a dedicated page on this website and a pinned thread on how to help, but with those templates we can have some kind of checklists for contributors (that they cannot dismiss and therefore would also reduce the amount of questions/out of scope feature requests open on github). This checklist could include instructions on what to do for promotion on the forum.

Rebating should have been rebasing, thanks autocorrect... Anyway, having CI also check the code after merging changes into master would help identify issues. I think it should be possible to configure a bot to report simple but helpful instructions for contributors (e.g. Please rebase your code).

Of course, if we agree on such changes in the infrastructure we would need people willing to help implement those changes. ;)

As for the tags: I haven't checked lately what options github gives us. Can we allow only tags to be set by a PR author, or everybody can modify them? I'm not 100% convinced it actually needed to let contributors change them, though. Currently we can ensure a consistent state with our github kanban. Also, what would be the added value for contributors? It's just a tag, helpful for searching, but not influencing the discussion happening in the PR.
SFML / OS X developer

eXpl0it3r

  • SFML Team
  • Hero Member
  • *****
  • Posts: 10801
    • View Profile
    • development blog
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #6 on: March 29, 2018, 03:09:45 pm »
I'm happy to bring some changes to our process in order to move things forward, however I think we should keep realistic goals.

The goal would be for any PR that don't need updates (ie. only review and testing time) to be merged within at most 2 days. This is of course not doable if parts of the process can be done by too few people, but totally doable if the process is scalable.
This is a very unrealistic goal, as we simply won't have the resources available right within that time frame to do in-depth reviews and testing on 3-5 different platforms.

In both cases my personal feeling is that currently the PRs that are blocked by a (code) review are not visible in SFML's PR list. The tags in https://github.com/SFML/SFML/pulls don't look to give this information.
To me it seems you're not aware of the GitHub Project (sort of Kanban board) that we (or mostly I) maintain: https://github.com/SFML/SFML/projects/2

Tracking the status of a PR or issue isn't always trivial. We have tried different labels in the past, but the issue list simply isn't very flexible. Ever since GitHub has introduced Projects, I've be using it extensively and it's a lot easier to track what is going on. There might still be room for improving the process and I might need to be more strict with moving cards around, but the important sections are "Ready" and "Review & Testing". As soon as a PR hits the "Ready" section it will be merged the next time I get around to merging things, unless someone in the meantime raises some concern and "Review & Testing" PRs require additional community attention.

Nobody talked of rushing :) only being more efficient to reduce the time between when the PR is opened and when it is merged. Which most likely implies sharing more work with the community.
What's the reason that "merging as fast as possible" is so important to you? When a PR is merged really isn't important, important is that reviews and testing has been done and there's a high enough confidence in the code changes.
Take your PR as an example, it's been merged now for a few days, what if the PR got merged just today, where's the difference and how is it so much more important to have it merged right the instance when reviews and tests are done?
As mentioned when something is moved to "Ready" it is as good as merged, but it also still leaves the PR open for a while, so people can do their final testing or raising some concerns. To me there's no real difference in progress tracking whether something is merged or in the Ready state.

But let's take my last PR as an example (although it's an extreme case) for which testing took 2 months, where probably 90% of that time is waiting for feedback. During that time I refrained from helping on other matters, because I didn't want to pile unmerged work up, I wasn't sure it would be merged at all, and it was rather demotivating to get sparse feedback. So what you seem to consider a good enough process means that I could have contributed more or less 3 times more to SFML in the same time range. Up to you to consider if it's worth it.
If this is reason you want to get stuff merged faster, then that's really just an issue on your end. If you don't want to deal with multiple PRs at once, then that's fine, but you can't just put the blame on the process. You could have contributed more or less 3 times more to SFML, if you used the waiting time to do it. An open PR didn't physically block you from contributing more. ;)
As for feedback, it just takes time, especially with such a large change. There aren't hundred of people just waiting to test and review and of the people that are there to help, only a handful will be familiar enough with CMake to give this a fair try. Then you have Windows, Linux, macOS, iOS and Android that these change affects in some way. Not everyone has access to devices that run these OS or have the knowledge to operate them properly.
In my books 2 months of testing and feedback is quite realistic for the given change set.

I really insist about the fact that the PR creator must be able to set the tags himself (if we go for the tags solution). It’s important that there is no need to wait for anyone from the core team to set the tag, and that we don’t wait for the PR to look blocked : the tag should be set as soon as the PR can be reviewed.
To my knowledge this isn't possible with GitHub.

I noticed that only eXpl0it3r is merging the PRs. Why? I would expect that at least all members from the SFML core team could do this. Also I wonder if there is currently a minimal wait time before merge (I remember there were notices like "If nobody is against this PR withing X days it will get merged" in the past).
As mentioned, merging is not a high priority task, as such there's no need to spread the responsibility further. Involving more people simply increases the overhead of having to align with each other, running into conflicts or someone forgetting a step in the process.
The times given were never really accurate and with the GitHub Project, this is now more or less implicit. It moves to Ready and within the next few days it will eventually be merged down.

1. eXpl0it3r is amazing at managing tasks, I wouldn't say he is the bottleneck, and I think having more people involved in this process would just make it a big mess -- unless eXpl0it3r is fed up and wants some help, of course :)
It's true that we have relied solely on eXpl0it3r for the merging procedure, and that for quite a while. He's doing a fantastic job for sure. But maybe, eXpl0it3r, do you want other teamates to help?
Awww thanks :)
I'm good so far and everyone from the team is free to prioritize issues they deem important or want to work on. I'll in the end make sure, everything is setup in a consistent way. And if I see community members getting interested in a topic, that will also be sorted in.

Currently merging is done in batches to make rebasing less tedious. If we merge a PR everyday or so, every other PRs need to be updated. So we have to find a tradeoff.
I think the rebasing isn't really needed as long as there are no merge conflicts, it's just that GitHub (and pretty nobody else where) supports a rebase-merge strategy like we do, as such if branches are rebased and merged, GitHub won't realize that the PR was merged, so I have to manually close it. But that's no real issue and I think, we shouldn't tell people to rebase, as it just annoys contributors and doesn't really add value.
The batching is less of a rebase issue and more of, when I sit down and actually merge stuff.
As for resolving merge conflicts, it often is the easiest if the original author does it, as they know what code they added and thus will most likely do fewer errors, but if there's no activity from the author, we can also do it.

To "fix" this we would need an easy way to see the list of PRs that are waiting for review. [...] or a forum section with a topic per PR needing review. This last solution looks maybe overkill but the advantage is that it's visible from all SFML forum members that would visit this section, and if people want to help on SFML development by reviewing, they can know where to help in an easy way. If we go this way it should be the PR's author's responsibility to open the topic in the forum section, so that it's not more burden for SFML core team.
We actually had an internal discussion about this many moons ago and sort of agreed to do it, but then never actually did it.

Should the (code) review be done always by at least a SFML core team member, or can it be done by any member?
[...]
When testing is needed, can it be done by anyone or should there be at least 1 tester from SFML core team?
We depend on the community to review and test. So far I have seen about 3 "GitHub" reviews, i.e. where someone started the GitHub review process. Would be nice if this number increases, even though we also do count "looks good" to some extend.
We absolutely depend on the community to test. Everyone has different hardware and many things just need that diversity.
I don't think we can have a fixed number on who has to look at what, because it really depends. For a minor change for example, it really doesn't matter if someone from the community tests or reviews this, but when you get API changing stuff, we really do want SFML Team members to sign-off on that.
As usual, the more help the better!

Conclusion
We need to keep it realistic and aim for time frames that make sense, which IMHO is very dependent on the task and the availability of people. Plus we need to consider that simply changing the process doesn't guarantee in any way, that suddenly more people will actually spent the time to review and test pull requests, but I think a bump of one or two people occasionally is already worth the investment, plus it might give a better visibility on what is going on with SFML's development.

Things we already have setup

Things that I think could be useful
  • Dedicated sub-forum for pull requests
  • GitHub issue/pr templates
  • Always providing proper testing instructions at best with example code

Other things that could potentially be done
  • Simple document on what the process roughly looks like
  • Bot to post links to artifacts, so people are more aware
  • More/different tags on issues and PRs
  • ...

Thanks for all the feedback and work, let's look forward to when PRs move forward faster! :)
« Last Edit: November 07, 2023, 11:46:39 am by eXpl0it3r »
Official FAQ: https://www.sfml-dev.org/faq.php
Official Discord Server: https://discord.gg/nr4X7Fh
——————————————————————
Dev Blog: https://duerrenberger.dev/blog/

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4321
    • View Profile
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #7 on: March 29, 2018, 07:11:41 pm »
If you don't want to deal with multiple PRs at once, then that's fine, but you can't just put the blame on the process. You could have contributed more or less 3 times more to SFML, if you used the waiting time to do it. An open PR didn't physically block you from contributing more. ;)

I disagree with this statement. Sure, one is not physically blocked to do so, but you'll have to agree that basing your work on top of an open PR implies dealing with a lot of rebasing and conflict resolution. This is by far not the interesting bits! In my previous work experience I had to deal with that. It's really demotivating. If we can speed the merge process a little bit it would be a win. I mean specifically that when the code review and tests are done (not before) we try to merge more quickly.

That being said, I agree with you when you say that it takes time to properly review and test things out. But for that, I see quicker turnaround if the community is truly involved.
SFML / OS X developer

Ceylo

  • Hero Member
  • *****
  • Posts: 2325
    • View Profile
    • http://sfemovie.yalir.org/
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #8 on: March 29, 2018, 08:40:32 pm »
[...] having CI also check the code after merging changes into master would help identify issues. I think it should be possible to configure a bot to report simple but helpful instructions for contributors (e.g. Please rebase your code).

Of course, if we agree on such changes in the infrastructure we would need people willing to help implement those changes. ;)
A reminder is still an improvement but a small one. What would really help would be the automatic test with merge. I guess it depends on the CI itself, so the first step would be checking whether the currently used CI system does support this. If we have to implement it ourselves it's not worth it to me.

Also, what would be the added value for contributors? It's just a tag, helpful for searching, but not influencing the discussion happening in the PR.
That's the point, easier searching. But no need to care about it if we go with forum solution or if we can get people used to the GitHub Project page.

This is a very unrealistic goal, as we simply won't have the resources available right within that time frame to do in-depth reviews and testing on 3-5 different platforms.
For sure we don't have the resources at the moment (or at least we don't see them) but I disagree when you say it's not realistic. Of course this is subjective so let's move on.

To me it seems you're not aware of the GitHub Project (sort of Kanban board) that we (or mostly I) maintain: https://github.com/SFML/SFML/projects/2
I've seen it but I don't consider it to be a usable tool unless any PR author can change it, which I guess you don't want so that you can keep control. Probably a matter of trust. Again the purpose is to make the list of PRs ready for review/testing visible immediately, not to depend on someone from the core team for that.

As soon as a PR hits the "Ready" section it will be merged the next time I get around to merging things, unless someone in the meantime raises some concern and "Review & Testing" PRs require additional community attention.
Could you give a ratio of the PRs where someone has actually raised a concern during this "Ready" stage? On all the PRs I've looked at I don't remember any where there was a concern, but maybe I just didn't take a representative sample.

What's the reason that "merging as fast as possible" is so important to you?
So as I already stated
Quote from: Ceylo
During that time I refrained from helping on other matters, because I didn't want to pile unmerged work up, I wasn't sure it would be merged at all, and it was rather demotivating to get sparse feedback.
Additionally here are a few other reasons:
- Dependency: when a PR is pending, other changes I do may conflict with my previous changes, and I can't always predict this and thus create my next branch from the pending PR branch. Also if I create the branch from the current pending PR branch, I risk basing my work on stuff that won't be merge. And I can't tell in advance whether the PR will be refused, although it's usually unlikely.
- Shipping the value to users: unmerged work is not available to users, so with slow merging you're constantly wasting time for the people having the issue that would be fixed by the PR. This is also valid for the PR author himself: if he fixed an issue related to the development environment, you're keeping him in that problematic environment

If this is reason you want to get stuff merged faster, then that's really just an issue on your end. If you don't want to deal with multiple PRs at once, then that's fine, but you can't just put the blame on the process.
Well if it's an issue for any/some contributors, then this is your issue. Because without contributions SFML dies. I'm just one contributor among many others, but I don't think telling "this is problem with you only" is gonna help with motivation and contributions :) .

As for feedback, it just takes time, especially with such a large change. There aren't hundred of people just waiting to test and review and of the people that are there to help, only a handful will be familiar enough with CMake to give this a fair try. Then you have Windows, Linux, macOS, iOS and Android that these change affects in some way. Not everyone has access to devices that run these OS or have the knowledge to operate them properly.
In my books 2 months of testing and feedback is quite realistic for the given change set.
Of course reviewing and testing takes time. But it for sure shouldn't need 2 months.


To "fix" this we would need an easy way to see the list of PRs that are waiting for review. [...] or a forum section with a topic per PR needing review. This last solution looks maybe overkill but the advantage is that it's visible from all SFML forum members that would visit this section, and if people want to help on SFML development by reviewing, they can know where to help in an easy way. If we go this way it should be the PR's author's responsibility to open the topic in the forum section, so that it's not more burden for SFML core team.
We actually had an internal discussion about this many moons ago and sort of agreed to do it, but then never actually did it.
Interesting to know!

I don't think we can have a fixed number on who has to look at what, because it really depends. For a minor change for example, it really doesn't matter if someone from the community tests or reviews this, but when you get API changing stuff, we really do want SFML Team members to sign-off on that.
As usual, the more help the better!
Ok.

Plus we need to consider that simply changing the process doesn't guarantee in any way, that suddenly more people will actually spent the time to review and test pull requests, [...]
What's sure is that not changing anything won't make more people come help :P

Things we already have setup
For curiosity, do you know if anyone is using the GitHub project apart from you? (NB: I'm not implying it doesn't make it useful, even if you were the only one using it)
« Last Edit: November 07, 2023, 11:46:21 am by eXpl0it3r »
Want to play movies in your SFML application? Check out sfeMovie!

Jonny

  • Full Member
  • ***
  • Posts: 114
    • View Profile
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #9 on: March 30, 2018, 02:19:55 am »
I don't have the energy to go through quoting/replying, but a couple of quick thoughts:


- Github project is great - definitely better to have than not (as long as Mr. Pl0it3r is happy maintaining) I actually think the GitHub systems have vastly improved over the past year or so (excluding my next point...)

- I know the SFML team have voted on this in the not too distant past, but I'd still reconsider the requirement to post on the forum before logging an issue. Github is a significantly better bug tracker than the forum, and is accessible to a much wider audience. I also believe it would help create a more positive dialogue between team members and contributors on GitHub (because tbh if I ever get the "go on the forum" response when logging an issue on GitHub, I simply won't bother). The forum search is also pretty infuriating...

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4321
    • View Profile
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #10 on: March 30, 2018, 08:55:00 am »
Quote
having CI also check the code after merging changes into master would help identify issues. I think it should be possible to configure a bot to report simple but helpful instructions for contributors (e.g. Please rebase your code).
A reminder is still an improvement but a small one. What would really help would be the automatic test with merge.
I'm not sure I follow. What I meant was that our CI would, in addition the existing tests, test the code after a local merge of the branch into master and, if any of the tests fail, the CI bot could emit a message on github with the error details.

Did you mean that the CI should automatically merge things? That'd be inadequate for the simple fact that the tests currently consist only in compiling the code. This won't indicate whether a bug was fixed or a feature correctly implemented.

Quote
I've seen it but I don't consider it to be a usable tool unless any PR author can change it, which I guess you don't want so that you can keep control. Probably a matter of trust.
It's not really a matter of trust, although if every kiddy can mess around our boards it would be a mess, obviously. But I think it absolutely makes sense to keep this board in the hands of a few. Why? Because, like a real company, not all developers are project managers and those are the only one that should organise priorities, IMO. The same applies, less critically, to tags in the end.

Quote
For curiosity, do you know if anyone is using the GitHub project apart from [eXpl0it3r]?
I do go and have a look from time to time, yes. Gives you a good overview of the progress and current status.

SFML / OS X developer

Ceylo

  • Hero Member
  • *****
  • Posts: 2325
    • View Profile
    • http://sfemovie.yalir.org/
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #11 on: March 31, 2018, 02:19:29 pm »
I'm not sure I follow. What I meant was that our CI would, in addition the existing tests, test the code after a local merge of the branch into master and, if any of the tests fail, the CI bot could emit a message on github with the error details.
I misunderstood, what you describe looks nice :)

Did you mean that the CI should automatically merge things? That'd be inadequate for the simple fact that the tests currently consist only in compiling the code. This won't indicate whether a bug was fixed or a feature correctly implemented.
No, I didn't intend to go that far.

It's not really a matter of trust, although if every kiddy can mess around our boards it would be a mess, obviously. But I think it absolutely makes sense to keep this board in the hands of a few. Why? Because, like a real company, not all developers are project managers and those are the only one that should organise priorities, IMO. The same applies, less critically, to tags in the end.
Well the developers could update the board to update status about what's ready to be reviewed, etc. Without changing priorities. Anyway it doesn't look like it could be used efficiently by PR authors to give visibility about PR needing testing/review so let's forget this.

[...] a couple of quick thoughts: [...]
I don't have any opinion on your ideas but thanks for the input!
Want to play movies in your SFML application? Check out sfeMovie!

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4321
    • View Profile
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #12 on: April 05, 2018, 07:54:28 am »
I've opened https://github.com/SFML/SFML-Buildbot/issues/15 if anyone wants to chip in!  :)
SFML / OS X developer

Ceylo

  • Hero Member
  • *****
  • Posts: 2325
    • View Profile
    • http://sfemovie.yalir.org/
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #13 on: April 05, 2018, 07:49:52 pm »
Thanks for pushing things forward Hiura.

@eXpl0it3r @Laurent
After a step back I'm really puzzled about the fact I had to convince you on "why" it is a good idea to have PRs merged more efficiently. And it seems like you're still not convinced. I would have expected that the discussion would only be focused on the "how" and not the "why".
Want to play movies in your SFML application? Check out sfeMovie!

Hiura

  • SFML Team
  • Hero Member
  • *****
  • Posts: 4321
    • View Profile
    • Email
Re: How to have PRs merged faster without lowering quality
« Reply #14 on: April 05, 2018, 08:06:39 pm »
Me too.  ???
SFML / OS X developer