Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update contribution guide and add issue/PR templates #64

Merged
merged 26 commits into from
May 31, 2024
Merged

Conversation

jonbarrow
Copy link
Member

Input from all major contributors need.

@DaniElectra @shutterbug2000 @SuperMarioDaBom @hauntii @gitlimes @ashquarky @PabloMK7 @InternalLoss @TraceEntertains

Not a member of the core team, but I'm also going to ping @MatthewL246 for input because he's been doing great work in areas like https://github.com/PretendoNetwork/mitmproxy-nintendo and his Docker-based project for our servers. Also going to ping @TheGreatRambler as he is a talented developer who has also contributed to important areas of our work, and as such his opinion is highly valued here.

Changes:

As discussed on Discord, this marks the start of the transition to issue and PR templates, and the enforcement of them and the contributing guide. What we decide to set here will become the basis for all other repositories, so we need to be sure we agree on things now rather than later. I've added some issue templates for bugs, regressions, and feature requests, as well as a pull request template.

Moving forward we're going to adopt a system similar to what @mrjvs used in his old project Movie Web (rest in piece). PRs are required (unless otherwise specified) to be linked to an approved issue. Enforcing this should help with the influx of PRs we have been getting, and making it so that contributors don't waste their time on something we aren't interested in. We value all of our contributors, and that includes their time, so preventing unnecessary PRs will be a big win.

(Most of the changes made by the core team will not necessarily need to go through this issue system. Though it would be nice to do so for transparency reasons, we tend to discuss changes via other means and agree on them outside of GitHub before commits/PRs are made. Thus making an additional issue for these changes is pointless)

The old contributing guide is also WILDLY out of date (written 6 years ago) and has NO mentions of code contributions, only how to setup consoles for development (which is now also wildly out of date and largely unhelpful). We need to set a new set of guidelines for code contributions. I am unsure right now to what specifics we all want, but I have left some starting points to go off of.

One of the most important things when it comes to contributions is consistency. With very few exceptions, we should stick to the same tech stacks for services. This also includes following consistent implementation patterns across servers, using the same style guides/linter rules, etc. Doing so will ensure that anyone can jump into any repository and easily navigate about it.

Some higher level points with that regard would be:

  • Game servers are written in Go
  • Game servers use Postgres
  • All other servers are written in TypeScript (with the exception of our Pokemon legality check server, which makes use for PKHeX and thus uses C#. This may change at some point, as we only need a small subsection of PKHeX which COULD be reasonably ported. But that's a conversation for another day)
  • TypeScript servers use Mongo
  • APIs are implemented using gRPC
  • Tools and libraries are written in languages which make sense (currently this has only really been Electron apps and TS/JS/Go libraries)

Though specifics can be discussed and/or set on a per-repo basis (though ideally what we set here should be able to be copy-pasted to other repositories with minimal to no changes).

@jonbarrow
Copy link
Member Author

Note that the PR is from a branch of this repo, so feel free to pull it down and add any contributions/changes

@jonbarrow
Copy link
Member Author

jonbarrow commented Apr 24, 2024

Another key point for the contribution guide would be the quality of commit messages. We have a staggering number of outside contributors who make changes with commits like updated file, or who seem to solely use the GitHub web interface for uploading files without changing the default upload commit message

When I did some work on https://github.com/typegoose/auto-increment in the past that project made use of the husky module to hook into git events. It then used those events to run a mix of lint-staged and commitlint to reject bad commits (both with bad messages and bad files)

A setup like this would be very beneficial to us, however it comes with some inherit drawbacks:

  1. Because it requires external tools, I'm not sure how it will behave with the commits made in the web interface (direct file edits, manual uploads, committing suggestions from PR reviews, etc.). It may or may not work
  2. These are all Node modules. They will work fine in our Node/Electron tools, but for everything else (like our Go game servers) this would essentially add the entirety of Node as a dev dependency for them. I know that it's likely for us to already have Node installed in these cases, but it's still somewhat annoying. Plus it would clutter our Go source with Node/JavaScript which, to be frank, is just plain ugly

I am unaware of any other tools we could make use of though, so maybe the pros outweigh the cons here and we should jump to using those tools?

Copy link
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seems mostly fine.

PRs are required (unless otherwise specified) to be linked to an approved issue

When we say "approved issue", does this mean that the issue must be manually verified by a member? Assuming it is, what are the exceptions that fall under "unless otherwise specified"? For example, I don't think a PR that fixes a typo should have an issue created first

Also, what is the path moving forward for existing issues prior to this? Will they be manually looked at to approve them or will they all be closed?

this would essentially add the entirety of Node as a dev dependency for them

Yeah I don't like adding node dependencies to repos that are completely unrelated to Node. Could this be handled using GitHub Actions instead?

The scale and scope of a commit should be reasonable. Do not commit for every line when making multiple changes, for example. However you should not include many unrelated changes in a single commit. By limiting the scope of the commit we can ensure that if any regressions or new bugs are introduced we can easily revert those changes without the need for major refactors or reimplementations. Limiting the scale of commits also makes review of the changes easier and faster.

## Messages
Commit messages should adequately explain the changes in the commit. Messages like "Updated file.md" and "spelling error" should not be used. Nonsense messages such as "oops" or "fixed" are especially not allowed. Commit messages should, at minimum, be in the format `type: message` where `type` represents the type or scope of the changes (`chore`, `docs`, `bug-fix`, etc.) and `message` is the actual changes. Unless the word is from the codebase and starts with a capital letter (such as an exported Go struct), the `message` should be lowercase. We also recommend using both "subject" and "body" commits. This can be achieved through the git CLI by using multiple `-m`/`--message` flags. For example `git commit -m "short subject" -m "longer description of the changes"`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the message should be lowercase

Unsure about having this as a requirement tbh. I personally prefer the subject being capitalized since it makes it stand out more from the description imo

The Linux kernel allows both lowercase and capitalized, can we also do this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section was mostly inspired by https://conventionalcommits.org/, which doesn't say messages should be upper or lower case. In fact it says either acceptable, it just says to be consistent. Given that currently our existing messages seem to always be lowercase that's just the ruling I went with here, to keep things consistent. If that was an incorrect judgement I'm open to changing it. The casing itself is not that important to me, just the consistency part

@jonbarrow
Copy link
Member Author

When we say "approved issue", does this mean that the issue must be manually verified by a member?

Yes. The issue templates I've set up automatically add a awaiting-approval tag for bugs/regressions. Once the issue has been manually reviewed and accepted then the accepted tag is added

This also helps for when I eventually add some sort of issue manager to the admin panel, so it can search for the tags across all repos

what are the exceptions that fall under "unless otherwise specified"? For example, I don't think a PR that fixes a typo should have an issue created first

This was referring to times when we've given approval for changes outside of issues, not a specific list of criteria. This was mostly meant for us, as a way for the core team to "get around" the issue requirement since we typically just either do a direct push or we talk about changes on Discord

I'm open to making a criteria list for this however. I originally wrote this with the intention of all changes, including stuff like typos, going through this issue approval process. But I can see how that might be cumbersome for no real reason

Also, what is the path moving forward for existing issues prior to this? Will they be manually looked at to approve them or will they all be closed?

Existing issues and PRs will be handled as if these guidelines didn't exist. Those people couldn't have known about these guidelines so there's no reason to punish them for it imo and make them redo their work. These rules would only apply after being committed

Could this be handled using GitHub Actions instead?

They can but it doesn't help with the issue. Using GitHub actions would only trigger errors after the commits happen. The point of linting commits is to prevent bad messages/staged files from getting into the history in the first place

I did find tools like this https://github.com/conventionalcommit/commitlint and https://github.com/llorllale/go-gitlint though, which are made for Go? This would solve our issue there but we'd be out of luck for anything not Node or Go (but how much of that do we have?)

Maybe we could use those? They seem kinda old, and look like they require some extra setup on our end, but they SHOULD work?

Copy link
Member

@ashquarky ashquarky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I don't mind 90% of this, though I've left some small nitpicks in - the document itself is also fairly long, might be worthwhile to trim it. That said, I am unsure about the linked issue requirement for PRs.

The barrier for entry to contributing to most of our projects is already fairly high - when I talk development with others, the most frequent thing they ask me about is selfhosting, which I still don't have a good answer for. I've started working on NEX only because I have admin panel access and can redirect servers around - otherwise I would have to host Accounts and Friends and a database and get them talking over gRPC and who knows what else.

I feel that requiring issues like this is just a workaround for the fact that we do most of our planning work in a private discord and aren't transparent about it. It's adding another barrier - filing an issue and getting it approved - for external contributors. We know this would be arduous and annoying because we explicitly waive it for internal contributions!

Sure, "talk to us first for big changes, we don't want you to waste your time" is sensible to have in there and other projects have it, but having it as policy for everyone feels very likely to turn off small-medium contributions, and imo anyone who knows our stack well enough to make large contributions is probably already on the core team.

In my opinion getting our internal plans out of pay-to-access channels and into public view would be much better for steering contributions, and would make people take e.g. the progress page on the website seriously. That puts the work onto us rather than drive-by contributors.

(While I'm on my high horse, I think getting some of the review work off Jon would help generally - we all do reviews, but outside of Juxt and the patches we usually wait on Jon to merge it in. I could see Dani handling NEX server PRs and Ash doing website stuff, for example, though ofc they would have to want to do that)


Before making a pull request ensure you have tested all changes and that no regressions have been introduced.

Pull requests should never be made against the default (`main`/`master`) branch of a repository. The default branch contains the most recent, stable, version of the codebase. All work on the codebase should take place in other branches. Unless otherwise specified, your target branch should typically be the `dev` branch. You may target other feature branches, however, if need be. If a `dev` branch does not exist for the repository you are working on, please submit a feature request for one to be added before continuing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have dev branches on Inkay at least, we all commit to main and make releases for the stable build. Should I learn a new workflow? Can this be enforced via branch protection/PR templates somehow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making a dev branch on all the repos is something we've been talking about for quite some time now (like a year or so). So this was going to be brought up either way

Your flow wouldn't really change all that much. Just working in dev rather than main and when a release is ready to be made merging and making the release. It's almost identical just with an extra step

Can this be enforced via branch protection/PR templates somehow?

I may be stupid, wdym?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, forcing PRs to be against dev rather than main.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this can be done via branch protection. There's already a handful of repositories in the org which protect the default branch from direct commits and require PRs, so that would be added as well

The point about targeting the dev branches was there not only to tell people not to even try and PR against the default branch, but to also not base their changes off of it (which isn't something we can directly control. The most we can do is tell them not to in a document like this, but we can't physically stop someone from making a branch to make changes based off the default branch locally)

# Making Issues
Before contributing any code, you should understand how we use issues and how to make them in a way which ensures you will get the response you want.

Issues are used by all repositories for most interactions. Issues are used for reporting actual issues with the codebase, as well as making feature requests and asking general questions. When making an issue, please use one of the provided issue templates. Using the provided templates keeps things consistent, as well as makes it easier for any of our 3rd party tools to interact with issues. If a template does not exist for what you wish to do, create a feature request for one. Not using an issue template may result in your issue being closed without completion.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a template does not exist for what you wish to do, create a feature request for one. Not using an issue template may result in your issue being closed without completion.

This feels like an unnecessary barrier to contribution, implying that they have to wait for a feature request to be implemented before even reporting their issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are issue templates for all the common use cases (even going so far as to split bug reports into bugs and regressions). The logic here is that if you need to use issues for something not covered by the existing templates then it shows we may need to add one

The point of the templates is to make it so that issues are structured the same across all issues, which should make reading them faster as we'd know where the relevant information is already

Do you have a better suggestion? (Genuine question)

Also to be clear the "it may be closed without completion" part was mostly aimed at people making issues that have templates available and just don't use them (like filing a bug report without using the template)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ask to please also file a feature request? So they can still file their thing immediately.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a small PR to implement a small fix - like, a typo in a print statement, do we really need an issue for that?

Do we need an issue with no comment or discussion for a PR that will definitely be merged? I feel like its unnecessary - issues IMO should be for 'these need to be implemented in the future but I'm not going to do it now'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well to start off with, "small fix" and "something definitely be merged" are 2 completely different things. Just because something is a small update does not mean it will be merged, at all. Take these PRs for example, both are very small changes which we were not interested in merging:

Secondly, thinking that your changes will "definitely be merged" is just wrong. You should never assume ANYTHING will 100% get merged. Even if the changes you make are correct, they may not be something we're interested in accepting at that point in time.

For example, the PRs you've been making to https://github.com/PretendoNetwork/nex-protocols-go. I brought this up the first time you made a PR, but the changes you're making will never be used, despite being correct and functional changes. This is because you're PRing game-specific protocols into the library, which is a pattern we're phasing out in favor of only implementing the "main" versions of protocols in the shared library and putting all game-specific code in the game server repository. So that makes us have to question whether or not we should even bother with review and merging those changes, since they will get trashed anyway.

Very similarly, I did not accept PRs into the website for a long time because @gitlimes had said they were doing a rewrite of it. Accepting PRs would create more changes that they would have to bring over to the rewrite, so I opted to pause all feature updates until that was done. Our stance has changed on that now, since @gitlimes has become too busy to finish the rewrite and @hauntii has slowly taken over that job, so now we do accept PRs for the time being. But this was not always the case.

Also: we're reasonable people here. Of COURSE if someone sees there's a single line change PR like a typo fix, for a repo accepting contributions, we won't outright reject that. It's ridiculous to think otherwise, tbh.

.github/CONTRIBUTING.md Show resolved Hide resolved
> [!NOTE]
> Developer documentation is highly work in progress. Do not expect to find everything there at this time. Please refer to the wiki first and foremost.

Finally, we recommend joining our [Discord server](https://invite.gg/pretendo) to see any roadmap plans or ask specific questions from our developers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Joining the discord is going to expose you to #pretendo-general, and it's really not a good way to reach out to us due to the flooding issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to alternatives if you have them. I couldn't think of any other real way to do this. So far Discord has been the best way to contact me, at least, and some devs have (devs wanting to use our servers for their projects or our tools). Genuinely asking, do you have a better idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be made a bit clearer that it is completely okay to ping a developer for genuine technical questions about the Pretendo servers. I didn't understand that until I joined the supporter chat and saw how willing you all were to answer questions because it isn't so visible in other channels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds fair 👍 if you have any specific suggestions for how to word this feel free to make a commit suggestion here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have had some luck redirecting technical questions into #support and having them still ping a dev, since it's at least easier to hold a conversation in its own thread. It's still not perfect because it won't get seen there unless OP finds the right person to ping, but it's better..

@jonbarrow
Copy link
Member Author

the document itself is also fairly long, might be worthwhile to trim it

I took notes from various existing contribution guides and this one is about the same length as most others (sometimes actually shorter)

That said, I am unsure about the linked issue requirement for PRs

The idea of requiring approved issues isn't a new one, and was shown to be very effective in other projects I've worked on. @mrjvs used it in their (now defunct) Movie Web project and it seemed to work fairly well

when I talk development with others, the most frequent thing they ask me about is selfhosting

This is just what happens when the project is many interconnected servers? You're going to have to figure out self hosting. But that also feels like a totally different issue? That doesn't really have anything to do with this document, at all? This is specifically about how to do code contributions, steps on how to setup a dev environment is out of scope here and would be put in its own repository?

I've started working on NEX only because I have admin panel access and can redirect servers around

You can do this as things are. I actually haven't hosted a single server outside of the one I'm working on in a while. All of my recent NEX development has been done by just using redirects. This again isn't really an issue within the scope of this document. Like I mentioned on Discord, the plan is to move all NEX servers to using domains rather than IPs, which some already do, and that would let anyone redirect to a local NEX server with a DNS change (which is how some of us already do it anyway)

If someone wants to work on something totally new, it stands to reason that yes they'd need to do some extra setup. But again different issue out of scope of this document imo

I feel that requiring issues like this is just a workaround for the fact that we do most of our planning work in a private discord and aren't transparent about it

No, that's not why. As I've said it's because people make PRs for changes we aren't always interested in. This wastes both contributor time and ours. It helps prevent people making "trash" PRs that make changes just for the sake of making changes. I HAVE seen multiple instances of all of this, and it's the driving factor to implementing this method of contributions

We know this would be arduous and annoying because we explicitly waive it for internal contributions!

No, again, that's not why. As I've said this isn't done because it's annoying, and it's not waived for us in general. The document says "unless otherwise specified", and in a lot of our cases we bypass it because we've already discussed the changes and we've agreed to them outside of issues. Therefore adding an issue is just pointless, since the changes have already been approved. If changes haven't been approved outside of issues (which, again, isn't specific to just us) then an issue would still be made. Dani also suggested some changes not needing issues at all, like small typos, which I did say I was open to discussing instances where changes don't need explicit approval

This isn't an "us" thing. It's a "the changes were already approved somewhere else" thing

I've seen several PRs that do things like updating deps in breaking ways without being tested, or making small useless changes that affect nothing, etc. Just "putting a roadmap out in the public" wouldn't prevent those kinds of bad PRs. It also wouldn't prevent PRs like we've seen on the website where some changes seem to be getting made to fit contributors personal preferences. Those kinds of PRs wouldn't be prevented by a roadmap

but having it as policy for everyone feels very likely to turn off small-medium contributions

We would disagree there, as I've seen this exact method work very well in other large projects (I literally stole the idea from mrjvs and even used his issue templates as a basis for ours)

In my opinion getting our internal plans out of pay-to-access channels and into public view would be much better for steering contributions, and would make people take e.g. the progress page on the website seriously. That puts the work onto us rather than drive-by contributors.

I agree with trying to get a roadmap out to the public, that's been the plan for a pretty long time already. But that doesn't fix the issues that these guidelines are intended to prevent

I think getting some of the review work off Jon would help generally

I can see where you're coming from, but there have been cases where stuff has been approved and then I've came in later and found issues. The most recent one being the PR you asked me to merge and had approved, but after looking at it again it had several issues (incorrect information, repeating text back to back, etc.)

@MatthewL246
Copy link
Member

I'm really glad that you're starting to prioritize allowing external contributions. As someone who was, until recently, very much outside of Pretendo development, I generally got the vibe that this project was open-source but not really open to external contributions.

A major reason for this is that development discussion is locked behind Discord channels that are either private or behind a paywall, and only some specific things get public on GitHub or the website/blog. Having a public and up-to-date roadmap would definitely help with letting people know what contributions are needed and being worked on, but I think the whole idea of most development discussions happening in private Discord channels is harmful to an open-source project. It would probably be unreasonable to somehow move all discussion into a public place that anyone can view anonymously, but I think it would be quite helpful to make the dev channels public (but still read-only) and create an additional few channels, like a development discussion channel and a forum for technical questions, specifically for external contributors to discuss development. Of course, most people in the server have no interest in contributing to development, so the dev channels should probably be hidden behind "Channels and Roles" by default. Avoiding off-topic stuff in the contributors' channel would be important, so perhaps it would also make sense to require a brief application through Bandwidth (similar to the current mod application) before being able to talk in the contributor discussion channels. Keeping off-topic channels for supporters only seems completely reasonable though.

The difficulty of self-hosting the Pretendo servers is definitely also a barrier to contributions, but at least that is just a technical problem that can be solved (and one that I am already working on).

Finally, I only have a couple of comments on the contributing document itself. Everything looks reasonable and quite similar to what other large projects do. It's clear that you are serious about maintaining the quality of external contributions. Regarding the commit guidance, I know that it is pretty common for even experienced developers to struggle with Git. What do you plan to do when someone inevitably uses bad commit messages and has no clue what an interactive rebase is? Requesting code changes is easy enough but requesting commit structure changes will get messy.

@ashquarky
Copy link
Member

Alright, if you think it'll be a net positive. I guess I'm just concerned about potentially making a difficult process harder. If it works for other projects I'm okay with trying it.
+1 to Matthew's idea of a free contributors channel, as long as we can moderate it fairly well to prevent it becoming general again. I think RetroArch has one of those, and there's a role you get to send messages.
Squash merges might be a workaround for bad commit messages?

@jonbarrow
Copy link
Member Author

I'm really glad that you're starting to prioritize allowing external contributions. As someone who was, until recently, very much outside of Pretendo development, I generally got the vibe that this project was open-source but not really open to external contributions.

We've always allowed contributions, there just isn't a lot of people interested/able to do so. Our work is fairly technical and requires quite a bit of specialized knowledge. Making that information more accessible is also another goal which we've been working towards for a while (hence our wiki and dev docs)

Having a public and up-to-date roadmap would definitely help with letting people know what contributions are needed and being worked on, but I think the whole idea of most development discussions happening in private Discord channels is harmful to an open-source project

Having a roadmap is, as stated, something planned. The big issue right now is that it just doesn't exist. We don't actually have a hard set roadmap, due to all work outside of mine being done on volunteer time. Things constantly change, and it's not feasible to make a real roadmap since stuff just happens when it happens. There are periods where days, or even weeks, go by without word from some people (which is fine!!! I never expect anyone besides myself to be generally available!!)

I think it would be quite helpful to make the dev channels public (but still read-only)

We likely won't be doing this, as much as I'd like to. It being a donor feature is one of the only things we could think of to offer people who support the project. Most people don't know this, but I was against donations at first because I didn't think we could offer people anything worthwhile. Removing that from people who paid for it is just unfortunately not something I think we can do

I do like the idea of opening up more plans and stuff, I'm just not sure how to go about it

and one that I am already working on

It's funny you mention that, the reason why mrjvs wanted to talk to you about your project is because I shared it in our general programming Discord server and I even mentioned wanting to ask you about making these tools official at some point. I genuinely love the work you've done and it solves very real issues

While likely not usable for our production deployments, it's genuinely helpful for local dev

What do you plan to do when someone inevitably uses bad commit messages and has no clue what an interactive rebase is?

Tell them to not do it in the future, direct them to this document, and carry on. Once it's done it's done, I wouldn't tell someone to redo all their work over it nor would I deny a PR from it. However if it happens repeatedly then that may be grounds for disallowing that user to contribute in the future (as it clearly shows they aren't reading this document)

We're exploring tools that prevent this before it even happens though. There's tools which hook into git events and run linters and other validation tools at the git level, with one of the most common use cases being linting commit messages (blocking commits with bad messages before they even happen) and validating staged files (to prevent files from being staged unintentionally)

But again if something happens and bad commit messages get through, then that's just that at that point

@jonbarrow
Copy link
Member Author

+1 to Matthew's idea of a free contributors channel

I'm open to an idea like this, but I don't think it would be right to just make the dev channels open to everyone. At the very least this takes away features people paid for, undermining people who donated just for that

Squash merges might be a workaround for bad commit messages?

Squash merges only help in a subset of situations but not all, and doesn't address the underlying issue. It doesn't help when a PR has several commits which do different things in related systems (a PR could contain typo fixes, general bug fixes, refactors and optimizations, etc. Each of those would be different commits and a squash merge couldn't adequately describe all of those). It also doesn't address to underlying issue and if we needed to go back through specific commits (which I've had to do several times over the past 7 years), it makes it difficult to track down the commits where specific changes happened. Checking git diffs and blames doesn't help there either when dealing with very large PRs

While I'm not against using squash merges, and the part of the guide which touches on PR titles being descriptive is part of that, I think both things can exist at the same time. Using squash merges doesn't inherently mean using bad commit messages

@MatthewL246
Copy link
Member

MatthewL246 commented Apr 26, 2024

It being a donor feature is one of the only things we could think of to offer people who support the project.

I do notice that judging by the role counts, there are significantly more people in the Super Mario tier than Mario. I'm sure you have more detailed statistics and I certainly can't read everyone's mind, but it seems to me that people consider access to beta games more valuable than viewing the developer chats and this wouldn't change that.

I'm also not sure whether the average non-technical supporter really cares all that much about the technical discussions or whether they just enjoy seeing the devs interact and being able to chat with you. Keeping #dev-offtopic private would let supporters keep the privilege of seeing the devs interact informally, and #dev-supporters would let supporters keep the privilege of chatting with the devs informally, as the contributor channels could be heavily moderated to only serious development discussion. Technical discussions could still happen in public without taking away what the supporters paid for. It could be a new set of channels if that's easier and you don't want 100% of everything public.

If you disagree with this, I would still propose creating some sort of public developer channel to at least get "I'm working on this"-type messages public so non-supporters can get more frequent updates on what devs are working on in place of a high-maintenance roadmap.

I even mentioned wanting to ask you about making these tools official at some point

It's great to hear that! That's something that's been in the back of my mind since I started the project. There are a few things I would want to do before making it official, like making better-organized self-hosting docs than the readme and upstreaming my server patches in general. I would be happy to discuss this.

> [!NOTE]
> Developer documentation is highly work in progress. Do not expect to find everything there at this time. Please refer to the wiki first and foremost.

Finally, we recommend joining our [Discord server](https://invite.gg/pretendo) to see any roadmap plans or ask specific questions from our developers.

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need to clarify which questions are more appropriate for a GitHub issue using the question template vs a discord question. Or just the user's choice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me tbh, unless anyone else wants to chime in about it? We may also want to hold off on this specific wording rn since the idea of new channels/a new server has been brought up. But atm this looks fine to me 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra context would have hugely helped me when trying to reach out. Given that this seems to be good advice now, could this be accepted then updated if there is a change to the public channels/server?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be accepted then updated if there is a change to the public channels/server?

The idea itself is pretty solid but we haven't settled on the best way to go about this yet, which is why this hasn't been accepted. The support channel is intended for people with general service issues, not necessarily technical discussions. I'm not sure that trying to reuse that channel specifically would do much good, as it would just start to clog the channel up a bit and wouldn't really solve the issue of getting developer attention (this was mentioned on Discord yesterday, but we tend to opt for a "community-first" style of support right now due to lack of staff. The community helps each other in those threads, and we only jump in when need be. Many of us don't even watch those threads ourselves, so any technical questions would have the same level of lacking visibility)

Honestly one of the biggest criticisms we get is people feeling like we rely on our Discord server for far too much (people dislike that it's one of the main ways to get support, since not everyone uses Discord). I've been thinking for a while now about just moving a lot of this out of Discord and into something like an online forum that we'd run. Moving stuff to our own forum on our website would solve several issues:

  • Support outside of Discord
  • Appeals for things like bans
  • More organized player reports
  • Asking technical questions

We can easily split these topics into their own categories on a forum to keep things organized, which would help with visibility a lot I think. I haven't brought this idea up with the rest of the team, but now is probably a good time

@DaniElectra @shutterbug2000 @SuperMarioDaBom @hauntii @gitlimes @ashquarky @InternalLoss @TraceEntertains @MatthewL246

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The support channel is intended for people with general service issues, not necessarily technical discussions.

It probably does make sense to keep these things separate. Let the community continue to help people who need more basic help with getting things set up or fixing errors, and keep the technical discussion in a place with a lower volume of posts so developers can keep track of them without needing to ping people.

moving a lot of this out of Discord and into something like an online forum that we'd run.

A forum sounds like a good idea to me. To be completely honest, I have no experience in contributing to online forums, but I really appreciate how they often provide great information about niche topics because they're public and indexed.

I'm conflicted about how organizing should work: I think that there are a lot of people on Discord who would be unwilling to sign up for an account on a forum, so it makes sense to keep the support channel available for them. (Not sure if any forum software has a "Log in with Discord" feature?) But the additional friction for joining a forum might help keep technical discussions higher quality? However, keeping the Discord support open also partially defeats the purpose of a forum, as people who don't use Discord will still be left in the dark about potentially useful information.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwilling to sign up for an account on a forum

There's no need, we have that solved already; the PNID. This isn't my first time doing this, actually. Around 10 years ago I was the lead of a massive vanilla MMORPG Minecraft server, and we had our own custom forum that I wrote which used users Minecraft accounts as their forum accounts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I didn't think about that, makes total sense!

@mrjvs
Copy link

mrjvs commented Apr 26, 2024

Since the success of the system in movie-web is mentioned quite often, ill drop in my evaluation of it:

Positives:

  1. It encouraged dialogue before a feature was being made with the contributor, allowing for giving more directed hints to an ideal solution. This resulted in less drastic requested changes in PR reviews.
  2. It's very easy to close PR's for features you don't want or feel doesnt fit the scope, since it simply doesn't have an approved issue

Downsides:

  1. The issue list needs to be kept up to date and regularly trimmed, at the times where we didn't do this at movie-web. It caused for lots of confused contributors. So regularly approve, assign and close issues and make sure that features in the internal planning are also in the issues. Missing issues mean nobody will contribute.
  2. You still get about 30% garbage PR's, it doesn't stop completely. People will simply not read the contribution guidelines. (However it is easy to close them with a simple documented reason)

Downside 1 can also be seen as a positive, since keeping your issue list updated is generally good practice.

Observations:

  1. There is no real place for incremental improvements. Think rewriting of documentation pages or code cleanup. These tasks usually don't have issues, so people who want to help out don't really have a means to do it officially.
  2. The features that had more complex setup, like needing the account server and client running at the same time, had significantly less contributions than the easier projects. So I would advice making services as easy as possible to run, minimizing the service dependencies. One way this was done in movie-web is making a testing CLI (as suggested by @jonbarrow), which has improved the amount and correctness of the PR's.

P.S. I agree with what @MatthewL246 suggested, having a tech discord channel is a good way for new contributors to ask questions about the code and/or how to test. movie-web had a good amount of healthy discussion in tech-general, a handful of the active members there turned into great contributors. It doesn't have to take away from the dev channels either, those can still persist and private. I find that opening channels of communication is an effective way to increase amount of contributors.

@jonbarrow
Copy link
Member Author

I appreciate your input, as the one who inspired this idea 👍 thank you for this

You still get about 30% garbage PR's, it doesn't stop completely

Definitely. I'm a firm believer that "the user will always find a way", so I don't believe in 100% fixes for most things anyway. Just mitigations as best we can. Plus as you mentioned, it gives us a good reason to close trash PRs without sounding like a jackass about it

Downside 1 can also be seen as a positive, since keeping your issue list updated is generally good practice

I agree with this view of it. Issues and PRs here have gotten a bit out of hand, so having something in place that encourages us to keep them trimmed and updated is a plus. I also planned to implement some tools into our admin panel to help manage issues and PRs (we have 126 repositories, so manually going through them all for issues/PRs just isn't feasible long term)

I didn't use you guys' method of assigning issues though, only approving them, for this same reason. Since we have so many repositories, and potentially many many issues/PRs, and I'm the only full time person, and there's just far fewer people who know how to contribute right now, it would make it difficult to properly assign people to issues. Most people here work on volunteer time, and I'd feel bad if I assigned someone basically unpaid labor. So once approved, anyone can work on them

There is no real place for incremental improvements. Think rewriting of documentation pages or code cleanup. These tasks usually don't have issues, so people who want to help out don't really have a means to do it officially.

What would you suggest here? If possible I'd like to keep everything within templates, for consistency reasons, and I'm open to making new ones (I've already expanded on yours by adding a specific regressions template and a general question template). I could see how this would fall under a bug report (documentation being out of date or incorrect is something I could easily classify as a bug), but using the bug report template for that does feel odd

So I would advice making services as easy as possible to run, minimizing the service dependencies

We agree, and it's actually already one of the goals we have outside of this document. I've tried to make services as easy to run as possible (even going so far as making the account server have several different options for caching, down to a "you didn't set anything so we'll just cache in memory" solution. I actually believe it was you jvs who kick started that push back in the day?)

That being said I feel like those are issues outside of the scope of this document, and would be better put in each services repository, yes? We're also working towards setting everything up with Docker, which should make it easier to spin up the services, and at some point I'd like to actually sit down with @MatthewL246 about making a dev env

(as suggested by @jonbarrow)

Off topic but I'm still very proud of that tool I made and I'm sad movie-web died, and it along with it

P.S. I agree with what @MatthewL246 suggested, having a tech discord channel is a good way for new contributors to ask questions about the code and/or how to test. movie-web had a good amount of healthy discussion in tech-general, a handful of the active members there turned into great contributors.

I agree with the idea fundamentally but I'm struggling to think of a way to do it comfortably. We've kinda backed ourselves into a corner here because access to our development channels is currently something people specifically pay for. I get notifications for all subscription status changes, and over time we have far more people subscribing to the Mario tier (which only gets you access to development channels, not the beta servers) than any other. It just looks like that's not the case based on the server roles because there's higher turnover, or people going from Mario to Super Mario (which has beta access) at some point

I'm not sure I want to deal with the PR of "Pretendo undermines supporters by taking away their exclusive perk". If that goes away then people on that tier straight up have nothing to gain and we're back at square one. I can also see people taking a move like that as "Pretendo trying to push people to pay more by moving perks to only higher tiers". So it's not as simple as just "flip the perms and give read access to our dev channels"

It doesn't have to take away from the dev channels either, those can still persist and private. I find that opening channels of communication is an effective way to increase amount of contributors.

Are you suggesting new channels then? Or a different server? I could be open to that idea, but I’m not seeing right now how they’d be different from the private channels? (like if they aren’t different from the private channels, it feels no different than just opening those channels directly y’know?)

@mrjvs
Copy link

mrjvs commented Apr 26, 2024

down to a "you didn't set anything so we'll just cache in memory" solution. I actually believe it was you jvs who kick started that push back in the day?)

I did give that push quite a while ago, glad to see it's still being pursued. I use these techniques both in open source projects and professional work and so far only seen benefits.

That being said I feel like those are issues outside of the scope of this document, and would be better put in each services repository, yes?

absolutely, my comment wasn't directly related to pretendo and this PR. It was my reflection and conclusion on the movie-web project management experiment as a whole, which I thought might be relevant in the decision making.

I also planned to implement some tools into our admin panel to help manage issues and PRs (we have 126 repositories, so manually going through them all for issues/PRs just isn't feasible long term)

Just a random thought, but I think you may be able to use github projects for it. Issues and PR's can automatically be added to projects. and project views can be filtered and setup to basically make an admin panel for it. I believe that was the vision behind the tool as well, multirepo management.

Github actions can be used for finer control, it can be triggered on pr reviews, issue creates, issue comments, basically anything.

Off topic but I'm still very proud of that tool I made and I'm sad movie-web died, and it along with it

It was a very nice tool, very useful for development. Eventually it was expanded to also include testing IP locked sources and even compiling for browser and running it with CORS restrictions. Made things super easy to test.

What would you suggest here? If possible I'd like to keep everything within templates, for consistency reasons, and I'm open to making new ones

Unfortunately, I don't really have an answer or suggestion for this part, it was just an observation of mine.

Are you suggesting new channels then?

movie-web's discord server had private developer channels and even more private project management channels. Those were the most active, but the introduction of a tech-general channel was enough to spawn some code related discussion. Just the introduction a simple generic channel like that can be enough, since it's an actual home for that kind of discussion.

I would discourage using tech-general for real developer planning and alike, but just let that new channel be an entrypoint for new contributors. I don't think that takes away from the value of the existing dev channels. (I think naming is important here too, calling it dev-github will make it a development only channel, but contributor-chat or tech-general will be less a dev channel and more a general chat for anything related to contributing for public members)

I would recommend asking supporters about it, ask them if the change would make them value the supporter tiers less. I don't know their answer and we won't know if we don't ask.

@jonbarrow
Copy link
Member Author

I have enabled this in all repositories and set it as the default configuration for new ones. I've also added a security policy to reflect this, so this should be taken care of.

@jonbarrow
Copy link
Member Author

I'm going to rename this repository .github after this merge, by the way, so that these changes get applied to all our repositories (as suggested by @gitlimes). When that happens though the pull request template will be "wrong", as it points to https://github.com/PretendoNetwork/Pretendo/issues?q=is%3Aopen+is%3Aissue+label%3Aapproved for the "appoved issues" link.

We can get around this by making the link relative, like ../issues?q=is%3Aopen+is%3Aissue+label%3Aapproved but that's kinda ugly? Maybe this link should just be removed entirely? I'm not sure there's a good way to do a link like this tbh.

@MatthewL246
Copy link
Member

I'm going to rename this repository .github after this merge

Would you like to move the README.md to /profile/README.md then? So it can appear as the organization profile.

@jonbarrow
Copy link
Member Author

Yes that was the plan

@gitlimes
Copy link
Member

gitlimes commented May 23, 2024

We can get around this by making the link relative, like ../issues?q=is%3Aopen+is%3Aissue+label%3Aapproved but that's kinda ugly? Maybe this link should just be removed entirely? I'm not sure there's a good way to do a link like this tbh.

I feel like that's an acceptable solution. The issues page will always be on that path, so I don't really see the ugliness tbh

@jonbarrow
Copy link
Member Author

We can get around this by making the link relative, like ../issues?q=is%3Aopen+is%3Aissue+label%3Aapproved but that's kinda ugly? Maybe this link should just be removed entirely? I'm not sure there's a good way to do a link like this tbh.

I feel like that's an acceptable solution. The issues page will always be on that path, so I don't really see the ugliness tbh

The ugliness comes from the fact that it's visually a relative link, and when you're on the page making the PR (which is where you're supposed to be verifying you're working on an accepted issue), the relative link isn't pointing to the issues page

@gitlimes
Copy link
Member

oh wait gotcha

* Before making a pull request, ensure the changes are for an approved issue.
* If your changes are not for an approved issue, your pull request can and will be rejected.
*
* CHECK https://github.com/PretendoNetwork/REPO_NAME/issues?q=is%3Aopen+is%3Aissue+label%3Aapproved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relative link?

Edit: This link can probably be removed if it is already in the checklist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link in the checklist is mostly meant for post-rendered use (so the link actually works). Using a relative link is not super clear at a glance, imo. So this comment gives a more visual indication of where to go

.github/SECURITY.md Show resolved Hide resolved
.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Matthew Lopez <73856503+MatthewL246@users.noreply.github.com>
@MatthewL246
Copy link
Member

Would you like to add an issue template config.yml file? You can add a link to the forum technical discussion category there so people see it when opening an issue.

@jonbarrow
Copy link
Member Author

Would you like to add an issue template config.yml file? You can add a link to the forum technical discussion category there so people see it when opening an issue.

Good call. Added that 👍

Copy link
Member

@MatthewL246 MatthewL246 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Member

@gitlimes gitlimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, Jon!

Copy link
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from this, this repository currently uses the MPL-2.0 license. Is that intentional? If not, it may be worth considering making a PR (not necessarily this one) to change it

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
jonbarrow and others added 2 commits May 31, 2024 10:43
Co-authored-by: Daniel López Guimaraes <112760654+DaniElectra@users.noreply.github.com>
@jonbarrow
Copy link
Member Author

Aside from this, this repository currently uses the MPL-2.0 license. Is that intentional? If not, it may be worth considering making a PR (not necessarily this one) to change it

I was going to say it doesn't matter because this repo has no code, but you're right that the default license should be AGPL. Updated 👍

@DaniElectra
Copy link
Member

The README should be updated too with the new license then

@jonbarrow
Copy link
Member Author

The README should be updated too with the new license then

This repository is going to be renamed to .github which turns it into one of GitHub's special repositories. The README will be used as the organizations profile README (https://docs.github.com/en/organizations/collaborating-with-groups-in-organizations/customizing-your-organizations-profile#adding-a-public-organization-profile-readme), so it's going to be completely rewritten anyway.

I was originally not going to worry about it, thinking that's out of the scope of this PR (which is just for contributions), but do you think it should be done here too?

@DaniElectra
Copy link
Member

In that case then I guess it's ok

Copy link
Member

@DaniElectra DaniElectra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jonbarrow jonbarrow merged commit acab547 into master May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants