Pull Request policy thread

Can’t this be accomplished with a tool like clang-format though? Or are you talking about variable naming, class design, and control flow?

5 Likes

Some aspects of naming (like camelCase, Classes starting with capital letters etc) can also be assisted by tools. Probably also for other aspects that Jules desires like applying a british spell-checker etc

Few key-points (imho):

  • The JUCE team are humans. there have been some times where a commit done by JUCE team broke things, introduced bugs, had inconsistent naming convention, etc…

  • The discussion/thread is about the policy rather than who’s actually pushing the change to the repo.

What feels that can be improved is the ability to have transparent and organized workflow.

In many cases, PRs on GitHub and forum posts simply decays over time. even one-liners.

To sum it up, It would be nice to have a transparent organized workflow agreed on JUCE with prioritization of topics.

11 Likes

I’ve submitted patches to clang-format to get it to approximate the JUCE style, but even the patched version doesn’t get it completely right some (most?) of the time. As an example, I don’t think that any formatter currently exists that would pass the “align = characters in consecutive assignment statements which are semantically similar, but don’t if they’re different” test.

Right, and I agree 100% that we should do a better job of replying to each PR and letting the owner know what we think of it, rather than just letting them feel ignored.

11 Likes

Thank you Jules for acknowledging that; that was the crux of my comments from the previous thread (JUCE Announces Acquisition by PACE)

I’ve already noticed some very positive improvement in the responsiveness to issues posted here on the forum, with links to the commits that address the various issues.

It would be great to spread this into GitHub PR’s, with a range from “Thanks but we don’t feel this appropriate” to “Here’s the commit we made based on your suggestion”.

5 Likes

If extending clang-format to get the JUCE style is too much work, would you consider modifying the style slightly to a style which is automatable?

5 Likes

I think clang-format is great, but at this point I’m not sure there’s a good way to go about “modifying the style slightly” in the JUCE repo. We want everything to look consistent, which would mean reformatting everything in one go, which in turn would introduce a giant “reformat the world” commit. Such giant commits are not very git-blame friendly…

An alternative approach would be to only format lines which are touched in new commits, but then you end up with slightly differently-formatted blocks in close proximity, which is a bit jarring to read. It becomes unclear whether the different formatting is trying to communicate something, or whether it’s just noise.

Additionally, I’m not sure clang-format is robust enough for our purposes. Different versions of clang-format sometimes produce different outputs, which is annoying if your package manager has one version and the build server runs a different version. In some cases, the formatting isn’t stable, so repeatedly running clang-format on the same snippet will cause the output to switch back and forth between two different layouts (only seen this once or twice, maybe it’s fixed now).

If we were talking about some new repo with no history, I’d definitely advocate for clang-format. For a large repo with a long history and which prides itself on consistency of style I’m not sure it’s such a good idea.

2 Likes

Git 2.23 added support for this. See Ignoring bulk change commits with git blame - Moxio | Software voor informatiemanagement for more details.

5 Likes

I recommend DeepGit for exploring the source of blame. Since using it I stopped fearing such commits since. I also know there’s some equivalent tools emacs plugins.

2 Likes

Seeing that so few people nail the style and there’s no desire for a formatter, and the culture for PRs is to let them amass and stagnate for the most part, the public really can’t do much more except spam the forums to always attempt putting an issue/bug/change on top of the devs’ plates - exactly like we’re doing today.

So this begs the question; CLA aside, what are the expectations for contributing to JUCE as an open-source repository?

I would like to suggest inviting collaborators help to do things like PR reviews, labeling, and maybe even creating Issues. There are different levels of collaborators, and Triage might do enough good to help out - seeing what we’re dealing with is a bandwidth issue.

This could help increase the culture in GitHub itself. I’ll volunteer.

4 Likes

if we had an army of reviewers who’d spend their time reviewing PRs for free, to a standard that satisfies us then that would be a great situation.

As a newcomer, one of the things I appreciate most about JUCE is the attention to detail and quality. I do think these things can be extracted, codified and community enforced — but that’s a lot of work, and like you mentioned, guaranteed to come with all sorts of additional overheads, like more community politics :slight_smile:

But engineering that is difficult, and our current system has been working really well for us so far. Maybe in the future things will evolve, who knows.

It seems hard to imagine that great PRs could pour in or that there could be an army of community reviewers as long as there are 2 development processes (private and public). The process the core devs contribute through will be optimized for by default, so it makes sense that it would feel higher quality and more productive.

I do wonder if it’s only when code review, CI, merging, etc all happens in public, that community members even have the chance to play on the same playing field, see great examples of what’s mergeable and what’s not, get the chance to level up, etc. Without those mechanisms in place, it follows that very few people would be able to deliver mergeable contributions. But also, the community is filled with C++ n00bs like me, so there’s that too :slight_smile:

However, the incentives of being a contributor and that feeling of having your code finally merged — weeee! These are important drivers for participation and learning. For example, I would like to add to the code documentation as I learn the framework, as there are some important contextual things missing I believe would help newcomers wrap their brains around best practices. But I’m not sure if the contribution is appropriate (are there public guidelines on docs?) and was a bit confused (until this thread) whether community contributions are truly welcome. (I’ll open a new thread on docs)

3 Likes

Can you show some examples of a PR that “works” but isn’t formatted correctly, thus causing the JUCE team to say “nah, that ain’t it, chief” and then how you would reformat it to match the “JUCE style”?

What is the objective here?
Is it to get your contribution in and getting a pad on the back, or that the feature gets added or a bug fixed?

1 Like

I think the goal would be to have an SDK that doesn’t require so many of us to maintain private forks

5 Likes

Why can’t it be a mixture of both? :slight_smile:

IME a PR usually begins with an issue or requirement that is not met, and in the ideal case ends up as a contribution and a feeling of achievement when it is finally merged in (whether directly or indirectly). Nothing wrong with that IMO.

Saying that, the JUCE team are generally responsive to request and bug reports and in my recent experience issues have been addressed fairly quickly, so I haven’t had the need to fork for a good while now. Of course others may have a different experience.

1 Like

I just had the impression, that some were arguing here their C++ proficiency. I understand that, I also am sometimes needy for appreciation and someone says “well done”. But hey, that should not be the motivation, and especially, if that is the reason for this thread, I might save me some time and mute it, because I don’t care who wrote what, as long as I can still understand the code and it is up to scratch.

The motivation is purely getting bug fixes and new features added. I couldn’t care less about a pat on the back. This is not about recognition. It’s purely about not having to maintain our own fork, if it can be avoided.

7 Likes

Once a new process has been established, I would advise closing ALL current pull requests and issues that are outdated and/or irrelevant, to give newcomers not the impression that the repository is a graveyard.

I think, from a business point of view, we can all agree that a “not perfect but working” pull request is better than a broken function or in our extreme case, a completely broken and unusable framework.

There is currently LOTS of very old code in JUCE that wouldn’t stand up to Jules’ current standards. We’re not deleting that either but keep it around to be improved at a later date. Adding a one-liner that helps hundreds but doesn’t suit the maintainers needs can be declared deprecated later, should it really be necessary.

Again: the perfect is the enemy of the good.

2 Likes

I agree that there needs to be some sort of policy in place.
At the very least, some kind of official response on the JUCE team’s opinion about the pull request, be it: “This is a good idea, adding it to the todo list” or “This is not a good idea because…”. At the very least, it would resolve every pull request that has been made and never acknowledged. If someone submits a Pull Request, they obviously found a solution to some problem they had with the framework that they think could help everyone else. It’s frustrating to never hear back from the JUCE team about these types of suggestions and forces us to maintain our own fork.

3 Likes