decomp.me icon indicating copy to clipboard operation
decomp.me copied to clipboard

[scratch][projects] Allow submitting PRs from scratches

Open matt-kempster opened this issue 2 years ago • 10 comments

Fairly untested right now and definitely needs more work, but just putting current progress out there.

Still needed:

  • [ ] Better error handling (incl. when scratch is not a project fn)
  • [ ] Only let users submit PRs for matched funcs?
  • [ ] Disallow PRs if context has been modified?
  • [ ] UI: we should probably hide this functionality for now
  • [ ] (after #392 is merged) raise public_repo scope if github API says we're not authorized to commit

matt-kempster avatar Feb 22 '22 00:02 matt-kempster

When PRing was discussed previously, we had talked about having a decomp.me account to PR matches and co-author the author. Is this being shelved for now?

JoshDuMan avatar Feb 22 '22 03:02 JoshDuMan

When PRing was discussed previously, we had talked about having a decomp.me account to PR matches and co-author the author. Is this being shelved for now?

@matt-kempster and I figured out how to use your github oauth token to make a fork of the project repo and create a commit as you, as if you had made it on github.com in the little web editor thingy. I think this is better and it doesn't necessitate a bot user

bates64 avatar Feb 22 '22 04:02 bates64

When PRing was discussed previously, we had talked about having a decomp.me account to PR matches and co-author the author. Is this being shelved for now?

@matt-kempster and I figured out how to use your github oauth token to make a fork of the project repo and create a commit as you, as if you had made it on github.com in the little web editor thingy. I think this is better and it doesn't necessitate a bot user

The fear we had when originally discussion was for needing full write permissions to decomp.me We figured certain users may have issues giving that significant of permissions to decomp.me. Does this oauth not have this issue?

JoshDuMan avatar Feb 22 '22 04:02 JoshDuMan

It does, yes. The live version of the site actually already requests the public_repo scope (see #390) and nobody appears to have brought up that as an issue :shipit:

This feature will be opt-in though, i.e. when you first try and make a PR it'll send you to GitHub asking for repo write permissions

bates64 avatar Feb 22 '22 04:02 bates64

I'm personally fine with it, and I imagine many people will be as well. I just worry that there may be some users with legitimate code they cannot risk trusting those perms at all,

Perhaps just add this onto that issue?

JoshDuMan avatar Feb 22 '22 04:02 JoshDuMan

IMO its fine for decomp.me's position on users with qualms such as those to be "okay, make the PR yourself then"

bates64 avatar Feb 22 '22 04:02 bates64

IMO its fine for decomp.me's position on users with qualms such as those to be "okay, make the PR yourself then"

Yeah this or we eventually can introduce the bot user to do it for you

ethteck avatar Feb 22 '22 04:02 ethteck

Disallow PRs if context has been modified?

I disagree on that point. Sometimes a function may indicate an issue within a helper function deeply buried within the context. If someone is able to find it, they should still be able to suggest (PR) that change. Maybe display a warning, so accidental changes don't happen too quickly, but it should still be possilbe at all.

MonsterDruide1 avatar Feb 24 '22 12:02 MonsterDruide1

Disallow PRs if context has been modified?

I disagree on that point. Sometimes a function may indicate an issue within a helper function deeply buried within the context. If someone is able to find it, they should still be able to suggest (PR) that change. Maybe display a warning, so accidental changes don't happen too quickly, but it should still be possilbe at all.

We want to allow editing of context, but these are implementation steps for version 1. we're trying to keep it simple and limit the scope of the feature for the first iteration

ethteck avatar Feb 24 '22 13:02 ethteck

marking this as a draft to visually distinguish it in the PR list since it's not ready

ethteck avatar Jul 23 '22 16:07 ethteck

This should be ready to review now. PR creation is currently limited to project maintainers (which, in prod, means @ethteck, @nanaian, and @JoshDuMan) and is done from project function pages:

image

If any of the attempts at the function are at 100% matched, an "Add to PR" button will appear on them. When this button is clicked, the scratch will be added to a basket on the bottom-right (on mobile, the top of the page). This basket persists across pages for that project, so you can add multiple scratches to build up a larger PR. In the basket is a "Create pull request" button that will:

  • Create a fork of the repository under your GitHub user if you don't already have one
  • Create a generated branch like decompme_GEN_asdfgh
  • In separate commits, update the relevant source files by replacing INCLUDE_ASM macros with the scratch source code (context changes ignored for now)
    • If the match involved forking, then parent scratch owners will be added as co-authors
  • Create a draft pull request

Example generated PR here

bates64 avatar Sep 13 '22 22:09 bates64