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

Allow submitting reviewed extensions via issues #1034

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

arthuro555
Copy link
Member

@arthuro555 arthuro555 commented Sep 27, 2023

The auto-pr system now reads the issue body to know whether the issue wants a reviewed or community extension submission and creates a PR with the file in the right folder.

Other misc improvements:

  • Many users seems to never see the next step of the submission, so they are now explicitly mentioned whenever the bot asks for changes and on the generated PR to ensure they are notified
  • The issue submitter will always be attributed correctly for extension, whereas before the event issuer's name was used leading to an extension team member's name being used if they labeled or edited an issue for a submission
  • The submission system will now correctly detect if a file is not a valid GDevelop extension AND if it is a GDevelop game file and give back appropriate error messages in these cases instead of crashing
  • Fix the auto-pr workflow running twice when a PR is created
  • Tag the PR with the type of submission (Community or reviewed)

I wrote unit tests and extensively tested the changes on my fork of the repository.

@arthuro555 arthuro555 added the ⛏ Build code Code that is used to build the extension database label Sep 27, 2023
@arthuro555 arthuro555 requested a review from a team as a code owner September 27, 2023 20:26
Comment on lines +54 to +60
// Basic check to see if it is a GDevelop extension
if (
!(
Array.isArray(extension.eventsFunctions) &&
Array.isArray(extension.eventsBasedBehaviors) &&
typeof extension.name === 'string'
)
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 check might be a bit fragile? If in we stop storing empty arrays for example if an extension only has a behavior and no free functions, a legit GDevelop extension might come out not being detected as a GDevelop extension.

@arthuro555 arthuro555 self-assigned this Sep 27, 2023
@D8H
Copy link
Contributor

D8H commented Sep 27, 2023

I really love the improvements you're doing on the submission workflow.

I have some concerns about using the "reviewed" folder automatically. I have the impression that for submissions asking for a review, the community list is often suggested because:

  • the extension requires more work to reach the "reviewed" bar and the submitter doesn't have time to do the improvements
  • the exact need is fuzzy and it needs experiments and breaking changes to be better focused
  • the scope is big and the reviewer doesn't have time to test and review it completely

It is also reassuring that the folder is "community" in case of mistake.

@arthuro555
Copy link
Member Author

Hmm... I hear you, how about this:

  • We continue allowing picking the review type at submission
  • We update the issue template the reflect the reality of the situation, maybe something like

Warning

A reviewed submission may take many months to be added to GDevelop, since it is a very long and tedious process that requires a lot of both your time and of the time of a voluntary reviewer.

We recommend you submit it as a community extension, which includes only a bare-bones review:

A full review will additionally require multiple quality assurance checks that you might feel are unnecessary, like:

  • Close inspection of the events being used to spot bugs or performance issues
  • Checking the comments of the extension to ensure they are clearly readable
  • Checking the wording of the events sentences for conciseness, correctness and typos
  • Checking whether the set of exposed actions, conditions and expressions makes sense or if some should be either removed or added
  • Etc.

You can always submit as a community extension first, then submit it again as a reviewed extension, allowing people to start using your extension while completing the additional review steps.

  • We remove the "duplicated" error message when an extension is submitted for another category than it's current one, replacing it by replacing the old community extension with a reviewed extension
    • This allows to create a "review" request by re-submitting as a reviewed extension an extension that has already been submitted as a community extension, all the while without having to learn or manipulate git
  • For people who first asked for a reviewed process then change their mind, we can add a PR command like !changetype community to make the bot change the PR submission type automatically

That way:

  • We strongly encourage picking community, explain why, and give an option to backpedal into a community extension after the review type has been picked
  • We still give the option of a reviewed extension if the submitter will not settle for anything less (useful for example for SDKs, integrations and libraries from companies that may want to show as having "official" support, not community support)
  • We still give the option to request a full review of the extension later on, after the community extension has been done, allowing for a quick and easy integration into the engine while still giving a non-gatekept (no need to know git and how to make a PR) way to get the extension to the next level

@george-gca
Copy link
Contributor

I believe the bot could also mention how to update the zip files in case they have to be fixed. I had struggled with this kind of thing since the bot doesn't say how to do that.

@arthuro555
Copy link
Member Author

That's fair! I thought the bot would write detailed explanation of the commands when creating the PR, but either I remember wrong or this has been changed 😅 I'll do that!

@D8H
Copy link
Contributor

D8H commented Sep 28, 2023

Hmm... I hear you, how about this:

* We continue allowing picking the review type at submission

Sure, I didn't suggested to remove it, just to keep the community folder for both as I've never feel the need to merge a submission to the reviewed list directly. Apart maybe some of the extension team extensions (which are direct PR), but it can be a good thing to start with the community list. I chose this path for some of my extensions.

The choice is still important to see the motivation.

* We update the issue template the reflect the reality of the situation, maybe something like

I fear that nobody will ever choose "reviewed" with a message this strong.

@arthuro555
Copy link
Member Author

arthuro555 commented Dec 9, 2023

Let's merge this for now, to get all the misc improvements this provides. If we want to implement a different flow where people must submit as community extension, then create a request for it to be promoted to a reviewed extension, we can change and implement the new submission flow as a part of another PR.

@D8H Is that alright with you?

Comment on lines -46 to +50
createWriteStream(`${extensionsFolder}/community/${file.name}`)
createWriteStream(
`${extensionsFolder}/${reviewed ? 'reviewed' : 'community'}/${
file.name
}`
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about asking submitters if they want to submit a update or a new extension?

  • a new extension would always target the community folder
  • an update would target either reviewed or community according to the extension to update.

The PR title or tags could make it clear that it's a reviewed extension update so that reviewers don't merge something in the reviewed folder by mistake without a full review.
It will also avoid to move extensions to the community folder because submitters chose "reviewed" whereas it's not up to them to decide and 90% of submission are likely to go to the community folder at least as a 1st step.

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'd love a system where we have an issue template to submit a new extension that won't allow using a name that's already in-use, andcreates a community extensions PR by default, with another issue template to later on request an upgrade to reviewed tier, and an update extension issue template that'll take into account the current tier of the extension you're trying to update.

That way, we encourage community extensions by default, provide an upgrade path to reviewed extensions, and can still submit updates to all types of extensions without risking overriding/"updating" another one by accident, or decreasing the quality of the reviewed extension demoting it to community extension.


With that said, this PR already has a lot of changes. I'd rather first merge this one for now, to already get these improvements out and figure out any potential bug with these changes, then work on reworking & improving the submission system even more.

Copy link
Contributor

@D8H D8H Apr 10, 2024

Choose a reason for hiding this comment

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

I agree with your goal, but I don't want to see PR adding new extension in the review folder while you implement the 2nd step because it will require me to manually fix these PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛏ Build code Code that is used to build the extension database
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants