jonbarrow
commented
2 months ago Note that the PR is from a branch of this repo, so feel free to pull it down and add any contributions/changes
Closed jonbarrow closed 1 month ago
jonbarrow
commented
2 months ago Note that the PR is from a branch of this repo, so feel free to pull it down and add any contributions/changes
jonbarrow
commented
2 months ago 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:
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?
jonbarrow
commented
2 months ago 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?
jonbarrow
commented
2 months ago 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
commented
2 months ago 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
commented
2 months ago 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
commented
2 months ago 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
commented
2 months ago +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
commented
2 months ago 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.
mrjvs
commented
2 months ago Since the success of the system in movie-web is mentioned quite often, ill drop in my evaluation of it:
Positives:
Downsides:
Downside 1 can also be seen as a positive, since keeping your issue list updated is generally good practice.
Observations:
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
commented
2 months ago I appreciate your input, as the one who inspired this idea :+1: 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
commented
2 months ago 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
commented
2 months ago I think you may be able to use github projects for it
I want to drop all of our use of GitHub Projects entirely. We already use it for task tracking, but it's a huge pain in the ass. It's super buggy at times and I don't enjoy using the GQL API. One of the biggest issues we've ran into is that if you link a project to a repo, and then rename that repo, it breaks the project and throws a generic GQL error when trying to query for it's data. Nothing short of deleting and remaking the project fixes this (we've had to do it MANY times)
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-generalchannel 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 like this idea. I'll pass it along internally too. Thank you for that!
TheGreatRambler
commented
2 months ago Would enhancement be a good tag for an issue that you want to write a feature to later PR for?
Also something that should be specified is the policy on squashing, since for testing one was likely making many small commits with poor names. It would be difficult to selectively choose what chunks of commits are suitable for being squashed. Generally when I PR where there is a policy on good commit names I squash every commit for that PR into one named similarly to the PR. What do you think?
jonbarrow
commented
2 months ago Would
enhancementbe a good tag for an issue that you want to write a feature to later PR for?
There's already a feature request issue template which would be used for that. However I do like the idea of an enhancement tag for incremental updates like what jvs mentioned earlier! That sounds like a good fit for that use case
Also something that should be specified is the policy on squashing, since for testing one was likely making many small commits with poor names. It would be difficult to selectively choose what chunks of commits are suitable for being squashed. Generally when I PR where there is a policy on good commit names I squash every commit for that PR into one named similarly to the PR. What do you think?
It sounds like you have more experience using squashes than I do (I've used them, but I don't make a habit out of it preferring just raw commits with good commit messages from the start)
I'm open to hearing some more specific ideas on that front from you as someone who already uses it in your work flow, feel free to leave some suggestions in the document itself. That sounds like a good thing to actually document :+1:
With regard to commit messages, the document does say that good commit messages should be used from the start. And when making PRs contributors are asked to verify that they read those guidelines. The point being to more easily find specific changes in larger PRs/squashed merges. I'm exploring tools to lint commit messages as well, nothing super serious just enough to be better than "Typo" for example. So ideally poor commit messages will be a rare occurrence anyway
jonbarrow
commented
2 months ago Aware that I've not actually contributed anything as of yet but thought I'd add some thoughts 😅
All opinions are welcome, and your first hand experience trying to jump in is objectively very valuable to us :+1:
Read through things on GitHub (which is difficult when, as you've pointed out there are a lot of repositories)
Unfortunately I'm not sure there's a good way to remedy this. We're just a large project with a large scope
Read through all support articles mentioning "contribution" and seeing that they end up at a bit of a dead end
This is both intended and unintended, tbh. We don't intend to ignore discussions like this, but that channel was originally designed for general service support and not technical questions. So in a way it's working as intended...? Due to the large size of our Discord server it's nearly impossible for us to ALWAYS be looking at support threads while also working on the project. So in our Discord we tend to let the community help in support threads first (we even have a dedicated role for this now), and we jump in when contacted directly. Honestly the support thread is not super ideal for technical discussions anyway
Additionally to that, not sure if it's worth having a single support thread (similar to the "Hacker Reporting" thread?)
I touched on this in one of the review comments but I'll restate it here for visibility reasons; I'm considering moving these types of things off Discord entirely. Migrating to an online forum on our website would give us far more control and better organization, and would almost certainly help with visibility
Trying to use git hooks to validate things can be frustrating, it's easily bypassed by contributors and won't be used at all if people ever make changes in the GitHub UI. In my experience, hooks can be good at aiding developer experience, they're not great at enforcing standards
As stated in a previous reply this is not intended to be the "end all be all" solution. It's a mitigation which does work in a lot of cases. The hooks and linting would be basic, just enforcing a general structure of messages and nothing else. I am aware that it does nothing for those using the GitHub UI, but that's fine. Like I said this was never meant to catch everyone. It's meant to be as good as a mitigation as possible
The inspiration for this comes from https://github.com/typegoose/auto-increment, which I contributed to some months ago, and the commit linting was generally a good experience imo. You can also see by the commit messages in the repo how it keeps them well organized and formatted
For our case I feel like simply enforcing a type: message structure is enough. Also with regards to bypasses, I am aware of this but having it both be in the linter and in this document would give us the ability to enforce these standards even when bypassed (when making PRs with these templates users must check off that they have read this document, and if it's clear they haven't or have intentionally bypassed it's rulings then we can correct the user. If the user continues to do so, we have grounding for rejecting further contributions as it's clear they do not care about our guidelines)
The linting and guidelines play 2 roles here. They play both the role of actually trying to act as an enforcer themselves, and when that fails they act as footing for us to enforce the guidelines if they are not respected
On the subject of squashing commits: I generally dislike this
Interesting. I don't have a strong stance either way on squashing (since you can still find the original commits in the PR, but ig that doesn't actually help when you take the codebase outside of GitHub :thinking:), but you're the first one I've seen be actively against squashing. I think I'll generally stay out of that side of the discussion, since I don't have a TON of experience with squashing in general (I've done it, but never felt the NEED to so I often don't). I'll let others here discuss that with you @mrjvs @ashquarky @TheGreatRambler (tagging those who I know use squashes). Also do feel free to actually leave some review comments and suggestions for this stuff :+1:
One thing that is good to clarify though is that not all PRs necessarily need to fully fix an issue
I thought this was implied given that the section on PRs doesn't say it has to fully close an issue, but we can make this more clear if you think it needs to be :+1:
jonbarrow
commented
2 months ago I've now added the enhancement issue template which can be used to request incremental changes to existing features. This should satisfy the issue jvs mentioned.
I have also clarified our stance on PRs not needing to complete issues, and have added a section about tests.
We do not currently have any strict rules on testing, and honestly most of our repositories lack tests completely. This probably should change, but I'm not sure how big of a priority this is at the moment.
ashquarky
commented
2 months ago Starting to get a bit hard to follow the separate threads here, so I'll just comment on a few things!
I am very in favour of some kind of dedicated space for technical questions and discussion (-> not # support), be it a discord channel, discord forum, or real forum. I don't think this takes away from what the Patrons are getting, given the core team sticks to the current dev- channels for talking to each other. I also think external contributors should be given the Tester role to see the backend channels (decided case-by-case of course). I know a lot of Linuxes have Discuss forums with support for their own OAuth systems, so I assume it can't be too hard to integrate PNID or Discord auth. Jon, I'm not sure writing a forum from scratch is a great use of your time, unless of course it's something you're excited about? I'm down to try a forum though, even the discord one we have internally is helpful. If you wanted me to suggest something, I'd say a Discord forum to start with - since that can be implemented immediately - and we can move to a proper forum later, once we have something with working PNID auth.
The reason I mentioned squashes is that the GitHub UI looks like this:
I can completely rewrite the commit message and description here, even if the PR is only one commit. This particular commit was a translation (non-code change), so not someone I expect to learn about rebasing. I assume docs and website changes could be similar.
Re: incremental changes, maybe we could just have a generic "Improve documentation", "keep dependencies updated" issue or milestone or whatever on all our projects? Could also apply the cute "help wanted" "good first issue" etc.
Example for Inkay:
This both gives an issue to link and gives a few suggestions for anyone browsing for something to do.
jonbarrow
commented
2 months ago I don't think this takes away from what the Patrons are getting, given the core team sticks to the current dev- channels for talking to each other
I agree! I think a forum is a great compromise here
I also think external contributors should be given the Tester role to see the backend channels (decided case-by-case of course)
I'm not super sure about this one. This feels like it starts to negate the compromise with the forum idea. Ideally those using the technical forum would become contributors, and if we give contributors access to those channels then why start with the forum at all? That feels like we've gone back to the original idea but with extra steps, and I'm worried will start to take away from supporters
Also there would HAVE to be some kind of criteria here. Even if we did go through with that idea I'm not sure people who do things like typo fixes, PRs to stuff like just the website don't necessarily need tester access for that? Those kinds of changes aren't exactly "technical" for lack of a better phrase. Not to discount those contributions, they just don't seem like they would benefit from seeing our more technical discussions
I know a lot of Linuxes have Discuss forums with support for their own OAuth systems, so I assume it can't be too hard to integrate PNID or Discord auth. Jon, I'm not sure writing a forum from scratch is a great use of your time, unless of course it's something you're excited about?
I am somewhat excited about it yes, and writing one from scratch would give us maximum control over everything. That being said you are correct that it's not a small task. And I wouldn't want to do it completely myself, as I'm no longer a frontend guy and I don't use modern frontend frameworks. I was going to ping @hauntii on Discord about it
That said, I was not aware Discuss allowed for custom oauth! I will DEFINITELY look into that! Tying into our existing account system was one of the biggest reasons for wanting to do it from scratch but this sounds promising
If you wanted me to suggest something, I'd say a Discord forum to start with - since that can be implemented immediately - and we can move to a proper forum later, once we have something with working PNID auth
We can also look into this yes. I'd want it to be a separate forum channel however, since one of the goals here is to help with visibility
The reason I mentioned squashes
I was thinking about squashes last night and I feel like we can compromise on them: allow squashes in cases where there's either minimal commits/changes (like in cases you mentioned with translation updates or single commit PRs), or where there's many related changes all together (like a PR that's just a bunch of small typo updates in one file). Otherwise for larger PRs with lots of changes, or PRs with many unrelated changes, no squash
Re: incremental changes
Are you suggesting these as all new templates? Just want to make sure I understand what you're suggesting before commenting on it. It feels like those instances would be covered under the "enhancement" template I pushed yesterday (it's intended for updating existing features, which can include docs and deps)
MatthewL246
commented
2 months ago I also think external contributors should be given the Tester role to see the backend channels (decided case-by-case of course)
This sounds like a decent compromise to deal with a slightly messy situation. I think "case-by-case" is the keyword here - not every contributor needs it, but some will find it extremely helpful. I'm not sure if there should be an application process or if you could just look for key elements in issues/PRs. For example, someone makes a decently technical/difficult issue and indicates that they want to work on it themselves, or they make a decently complex PR. (Of course, I'm biased here as someone who would stand to benefit from this.)
This feels like it starts to negate the compromise with the forum idea.
I don't agree with this at all. Viewing the dev channels has a completely different purpose than the forum: it allows people to see what you're working on and, if they have problems, search for old messages to see if you already had them. The forum is for asking new questions. Contributors asking questions in #dev-supporters should be encouraged to re-ask their questions in the forums to get an answer, this should catch on after a bit.
be it a discord channel, discord forum, or real forum
It might be useful to have a contributor general discussion channel on Discord as well as the forums. Discord chats and forums are just completely different formats, and Discord is convenient for general chats. Anyone asking a technical question there should be encouraged to ask in the forum instead.
I was not aware Discuss allowed for custom oauth
Hopefully, that will be much faster than writing your own forum from scratch. It would be nice to get it online by this summer - might be a great time to get new contributors with students (including myself) going on break.
If you wanted me to suggest something, I'd say a Discord forum to start with
I'm against this - if people get used to using a Discord forum, it might be more difficult for people to switch, and this just continues the issue of locking important information behind Discord. I know there are tons of blog posts already describing why this is a problem. To summarize, information on Discord:
All that being said, if you are set on writing your own forum from scratch and this is a task that is going to take months, a Discord forum is an acceptable compromise.
jonbarrow
commented
2 months ago To be clear, I am open to being convinced on things and I typically operate under a "majority rules" vote system. So do not take anything I say/ask as being negative, I mean everything I ask as genuine questions. I just wanted to clarify that before going any further as there's a history of misunderstandings when I interact with people online. And if I am out voted, I will accept that.
(Of course, I'm biased here as someone who would stand to benefit from this.) ... Viewing the dev channels has a completely different purpose than the forum
What benefits would people stand to gain that would not already be solved by other parts of this proposal? The idea being beneficial has been brought up already, but only vaguely without any real details about how that works compared to other proposals here.
The benefit of "knowing what we're working on/looking for" is solved by this PRs initial goal; by having people first make issues before making changes, we can open a dialog with them in said issue about whether or not the topic of the issue is something already being worked on/something we are interested in.
The benefit of being able to contact the core team is solved by bringing discussions into the online forum.
So I'm not sure what ACTUAL benefits people would gain from this idea that they don't already have in some form already (once this document is finalized and a forum plan established)? You mention past messages, but what would people gain from that on a technical level?
Contributors asking questions in #dev-supporters should be encouraged to re-ask their questions in the forums to get an answer, this should catch on after a bit
Why have this start in Discord at all, then? If the idea is to still push people to the forums, imo we should just cut the middleman out and keep all of that on the forum from the beginning. Splitting things between platforms becomes confusing and annoying to deal with. This would sometimes happen within our core team as well, which I pushed against. In the past it was very common for us to make PRs, and then have discussions about the changes split between dev channels and the PRs review. This makes it frustrating to keep track of, and so I pushed to keep all discussions in the reviews. I feel the same way about this, for non core team member technical discussions, that should probably stick within the forum.
It might be useful to have a contributor general discussion channel on Discord as well as the forums. Discord chats and forums are just completely different formats, and Discord is convenient for general chats. Anyone asking a technical question there should be encouraged to ask in the forum instead.
I agree they are different formats, but what kind of discussions do you see happening that wouldn't fit within the scope of the forum?
Hopefully, that will be much faster than writing your own forum from scratch. It would be nice to get it online by this summer - might be a great time to get new contributors with students (including myself) going on break.
I have pinged the relevant people on Discord about this already, waiting on a response and then we can go from there. While I wait I'll look into Discuss (truthfully I've only ever seen Discuss used as, like, a comment section on wikis. I wasn't even aware it had a forum).
I agree that if we decide to do it from scratch it should be done quickly (ideally a forum solution, of some kind, is shipped before even this PR is merged so it can be added to the document), possibly even becoming a large priority. I also want to get our developer documentation done relatively soon for the same reason but there's an incredible amount to document and only, really, 1 other person (@DaniElectra) who could reasonably contribute to it right now (since it was mainly he and I who designed the current versions of the core libraries).
I'm against this
Those are some very good points I had not considered
jonbarrow
commented
2 months ago @ashquarky Did you mean https://www.discourse.org/ by the way? I can't actually find any "Discuss" forum software. I assumed you meant https://disqus.com/ at first but looking at them again confirmed what I had originally thought, that they only did comments not forums?
jonbarrow
commented
2 months ago I will continue under the assumption that Discourse is what was meant originally.
I have looked into it, and Discourse DOES allow for external auth systems! This can be done both through their SSO system (called DiscourseConnect) or apparently through a totally custom plugin (though I've seen this not recommended for custom auth)
I have looked at DiscourseConnect and unfortunately it seems like that is not an option for us. DiscorseConnect uses the users email address to map between the external auth system and Discourse users, which means they assume emails to be unique (which is a good assumption!)
The issue is that emails are not unique in Nintendo Network, and are not unique here either. Nintendo allowed unlimited reuse of emails (I have dozens of NNIDs with the same email address), and so we also allow this. Meaning Discourse can NOT use emails to map to users, as emails may be reused by multiple people.
We may be able to get around this by forking Discourse (it's open source), but this would mean we lose updates without managing possible conflicts and would be a lot of effort for a simple integration. We can also fake email addresses by appending the users PID to the email address, which WILL make it globally unique, but will likely break all systems that rely on sending emails (like notifications if they do that by email)
DaniElectra
commented
2 months ago How about using discourse-oauth2-basic instead? It doesn't seem to require an email address
jonbarrow
commented
2 months ago How about using discourse-oauth2-basic instead? It doesn't seem to require an email address
Amazing find, this looks super promising
jonbarrow
commented
2 months ago @DaniElectra Upon further inspection it seems like even with this plugin, Discourse STILL assumes a unique email. If an email is not provided by the oauth provider the user will still have to submit it separately. Also there are posts mentioning the possibility of conflicts trying to reuse an email
The Discourse setup docs also make it a HUGE point to note how much they rely on emails, even going so far as to say that not setting up emails will break a deployment entirely
I may try to setup a local instance of Discourse to try and see how this plugin handles login. I'm worried that if Discourse relies this heavily on emails, then it may require emails to be sent at login. Which would not be compatible with our auth setup, as we wouldn't know which user is being logged in
ashquarky
commented
2 months ago I was indeed thinking of Discourse and not Disqus, my bad.
Suggestion: We could allow only PNIDs with verified emails to log in? If a dozen accounts all go to the same verified email, I think it's safe to assume they're all alts of the same real-world person, and can map to a single forum account.
MatthewL246
commented
2 months ago What benefits would people stand to gain that would not already be solved by other parts of this proposal?
You make a completely valid point here - the entire point of these proposals is to make it unnecessary for people to see the dev chats if they want to contribute. The main benefit for me was being able to search through past messages for potential answers to questions before asking them, or even just information to ask a more informed question. However, this could be solved by searching and asking questions in the forum.
To speak more generally here, the main point of this and some other recent projects like the developer docs is to move knowledge from private spaces to public spaces. If a contributor finds an answer to their question by searching the dev chats, then they won't ask that question in the forum. So, giving all contributors access might even be harmful to the end goal of making more knowledge public.
Why have this start in Discord at all, then? If the idea is to still push people to the forums, imo we should just cut the middleman out and keep all of that on the forum from the beginning.
I think you misunderstood my point. It was not that questions should be answered on Discord and then posted to the forums after, it was that people who ask technical questions on Discord should be told "No, Discord is not where we answer technical questions anymore, you should ask that in the forum and we will answer there." So, I think we're in agreement here.
I agree they are different formats, but what kind of discussions do you see happening that wouldn't fit within the scope of the forum?
I will say, I wrote this before my points against using Discord, and I ended up contradicting myself. In the end, keeping everything in the forum is probably better for organization.
Meaning Discourse can NOT use emails to map to users, as emails may be reused by multiple people.
Could you use PNID@pnids.pretendo.cc as the Discourse email? Then, forward anything from Discourse to the email associated with the PNID. You could run a small local SMTP server that takes in the emails from Discourse, changes the destination email to the PNID's verified email, and sends it along to SES.
jonbarrow
commented
2 months ago Suggestion: We could allow only PNIDs with verified emails to log in? If a dozen accounts all go to the same verified email, I think it's safe to assume they're all alts of the same real-world person, and can map to a single forum account.
The issue with that becomes which PNID to use. When using a custom oauth system like this, the users account information (username, profile picture, ect.) are taken from the external source and used for the forum account. In this case if there's 2 PNIDs both with a verified email, then which one would be the one that gets to "become" the forum account? And what if someone just genuinely wants to have multiple forum accounts for their existing PNIDs?
I have created a support thread on Discourse about this topic, if anyone would like to check it out and/or keep up with it over here https://meta.discourse.org/t/integration-into-custom-auth-system-where-emails-are-not-unique/306489
jonbarrow
commented
2 months ago To speak more generally here, the main point of this and some other recent projects like the developer docs is to move knowledge from private spaces to public spaces
To be clear, stuff like the information in the dev docs was never private. That information could always be found in our repos. And specifically our Go repos, one feature of Go is it's automatic documentation generation. The Go team maintains https://pkg.go.dev/ which scans and builds documentation for open source Go projects using Godoc comments. You could always find "docs" on our libraries there, for example https://pkg.go.dev/github.com/PretendoNetwork/nex-go
The point of projects like the developer documentation is to address issues we have with https://pkg.go.dev, specifically it's lack of customization and the inability to provide robust usage examples without cluttering the source files themselves. The developer docs also tie in with our wiki (which has also been public for a while) in order to give actual details about when to handle stuff like RMC methods, and what the functionality of the method should be internally. Take this page on DataStore::RateObjectsWithPosting for example. It describes all the parameters and how to handle the method.
So, giving all contributors access might even be harmful to the end goal of making more knowledge public.
I agree. It would be better to just have everything like that happen in the forums. That way it can be accessed more freely and can be found more easily by search engines
So, I think we're in agreement here.
We are, yes. I did misunderstand, though not in the way you thought. I thought you were suggesting to basically use our dev channels as docs, which is not something I agree with, and then just have users read stuff there and then post to the forums. When in reality docs should be the docs, and the forum should be for discussions. So I do believe we agree now :+1:
Could you use
PNID@pnids.pretendo.ccas the Discourse email? Then, forward anything from Discourse to the email associated with the PNID. You could run a small local SMTP server that takes in the emails from Discourse, changes the destination email to the PNID's verified email, and sends it along to SES.
We could, and that has been proposed already in my original reply about this issue. But that has 2 issues still:
MatthewL246
commented
2 months ago Ah, I had thought the Oauth would be a "Sign in with Google"-type button that would redirect to PNID sign-in and the fake email could be invisible to the user.
jonbarrow
commented
2 months ago From my understanding it completely replaces the default login method used by Discourse. At least, that's how DiscourseConnect functions. The oauth2 plugin did not specify either way, but I assume it just works the same way
wolfendale
commented
2 months ago I'm trying to understand exactly what benefits a forum brings if you follow the other measures that everyone has agreed on so far.
We do not currently have any strict rules on testing, and honestly most of our repositories lack tests completely. This probably should change, but I'm not sure how big of a priority this is at the moment.
I'm a big fan of automated tests and practices like TDD. I also think that adding tests to an existing codebase can be a really great way to understand it, so I'd happily be involved in looking into this, are there any repositories that you think would be best to start looking at?
jonbarrow
commented
2 months ago I'm trying to understand exactly what benefits a forum brings if you follow the other measures that everyone has agreed on so far.
Also the forum is designed to allow us to be reachable outside of Discord, in general. Not just for technical discussion but also for stuff like general support, appeals, etc. We DO rely way too much on Discord as a platform for these things and it blocks a lot of people from us for that reason. We have almost 300k users, yet less than 50k Discord members.
are there any repositories that you think would be best to start looking at?
Honestly, any of them. The only repository that has actual tests, to my knowledge, is https://github.com/PretendoNetwork/miiverse-api and it's just some scuffed postman tests. There's a couple small libraries which have tests like https://github.com/PretendoNetwork/boss-crypto, but those aren't REALLY "tests" in the traditional sense, imo. They're mostly just "sanity checks" (which I think are fundamentally different from actual unit tests), or just scuffed test scripts I used during development (they're closer to examples than tests).
A few of our database libraries like https://github.com/PretendoNetwork/pq-extended and https://github.com/PretendoNetwork/sql-manager have tests, but they're super minimal due to the simplistic nature of the libraries.
One of the issues we run into is that some stuff is just really hard to test. Like our networking libraries, or RMC implementations. You would essentially need a full blown custom client to run tests for those codebases, which is an idea we've toyed with doing but never really settled on.
So please, do feel free to take a look around at basically any repository if you're interested in making some tests. I'm sure you'll find some in desperate need of them. Again though, I'm a firm believer that 100% code coverage is both essentially useless and a waste of time. I'm not sure if you're familiar with him, but I honestly have essentially the same opinion as ThePrimeagen when it comes to tests and code coverage. So you can be conservative with them.
ashquarky
commented
2 months ago Do we think that the issue templates are likely to change much when this gets merged? I'd like to start drafting some feature requests for Inkay to get them out of my head.
jonbarrow
commented
2 months ago Do we think that the issue templates are likely to change much when this gets merged?
Probably not. I tried to make them as generic as possible so that they can just be slapped into all of our repos as-is once this is merged
MatthewL246
commented
1 month ago Are there any plans to add information about privately reporting security vulnerabilities? Public issues do not seem like the right place for that. GitHub suggests a SECURITY.md file, but I'm not sure if that can be set globally for the organization. Another option is adding an external link to the issue template config.yml.
jonbarrow
commented
1 month ago Are there any plans to add information about privately reporting security vulnerabilities? Public issues do not seem like the right place for that. GitHub suggests a SECURITY.md file, but I'm not sure if that can be set globally for the organization. Another option is adding an external link to the issue template config.yml.
I wasn't aware of that kind of file, that looks really helpful. As for the actual question, I haven't given that much thought to be honest. We don't really have a place to report anything like that atm? I'm open to suggestions however. I agree that using something public isn't a good idea however.
Also afaik all of this can be made global, yes
MatthewL246
commented
1 month ago Good ideas:
.well-known/security.txt file, more information is at https://securitytxt.org.Bad ideas:
jonbarrow
commented
1 month ago I'll look into the GitHub one. Thank you for pointing that out!
jonbarrow
commented
1 month ago
- GitHub has a private vulnerability reporting feature that you can enable everywhere as an organization admin. I have never used it, but it seems simple to set up and exactly what you want.
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
commented
1 month ago 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
commented
1 month ago 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
commented
1 month ago Yes that was the plan
gitlimes
commented
1 month ago We can get around this by making the link relative, like
../issues?q=is%3Aopen+is%3Aissue+label%3Aapprovedbut 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
commented
1 month ago We can get around this by making the link relative, like
../issues?q=is%3Aopen+is%3Aissue+label%3Aapprovedbut 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
commented
1 month ago oh wait gotcha
MatthewL246
commented
1 month ago 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
commented
1 month ago 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 :+1:
jonbarrow
commented
1 month ago 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 :+1:
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:
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).