Skip to content

[WIP] Enhance pull request template with detailed contribution guidelines and verification steps#21481

Draft
dwelch-r7 wants to merge 1 commit into
rapid7:masterfrom
dwelch-r7:improved-pr-template
Draft

[WIP] Enhance pull request template with detailed contribution guidelines and verification steps#21481
dwelch-r7 wants to merge 1 commit into
rapid7:masterfrom
dwelch-r7:improved-pr-template

Conversation

@dwelch-r7
Copy link
Copy Markdown
Contributor

This is just an initial attempt at improving the PR template to help reduce friction when it comes to both reviewing PRs and contributing PRs to the project.

I've marked it as a draft as I expect there to be some discussion on this for a while before we nail down exactly what we want this to say but I thought it was better to start with a rough template we can edit rather than starting off with a blank slate

@smcintyre-r7
Copy link
Copy Markdown
Contributor

I like it. The use of drop downs also keeps things organized though I don't know how often it'll be rendered because IIRC the default view when the PR is opened is the editor. Happy to keep it regardless.

One question for you/the team is would we find it useful to require disclosure of AI assistance, e.g. https://github.com/rommapp/romm/blob/405f67851423666040cc0e909e93b942c4e6d111/CONTRIBUTING.md?plain=1#L11-L32

@dwelch-r7
Copy link
Copy Markdown
Contributor Author

I like it. The use of drop downs also keeps things organized though I don't know how often it'll be rendered because IIRC the default view when the PR is opened is the editor. Happy to keep it regardless.

One question for you/the team is would we find it useful to require disclosure of AI assistance, e.g. https://github.com/rommapp/romm/blob/405f67851423666040cc0e909e93b942c4e6d111/CONTRIBUTING.md?plain=1#L11-L32

I originally had that in there and removed it, I figured we need to keep all code to the same standard whether or not it is AI generated so I don't think it's information that would be actionable for us, but I'd be happy if we wanted to add something like that in so long as we have some idea for what we intend to do with that information

@bwatters-r7
Copy link
Copy Markdown
Contributor

I like it. The use of drop downs also keeps things organized though I don't know how often it'll be rendered because IIRC the default view when the PR is opened is the editor. Happy to keep it regardless.

One question for you/the team is would we find it useful to require disclosure of AI assistance, e.g. https://github.com/rommapp/romm/blob/405f67851423666040cc0e909e93b942c4e6d111/CONTRIBUTING.md?plain=1#L11-L32

I feel like that take is a little more derogatory(?) toward AI than I think we are. If we throw in an AI disclosure policy, we should be a little more accepting of AI? I like the idea of knowing parts of a PR are AI-generated, but in the end, if it works, is tested, and it is not complete garbage altering 12 libraries for no reason, I'm cool with it.
That said, if it looks like the test output is AI-generated I'll kick shins. :rage1:

>
> - A clear **description** of what the change does and why
> - Specific, reproducible **verification steps**
> - **Test evidence** (console output, screenshots, or recording links)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NOT AI-GENERATED

- [ ] Ran `rubocop` on changed files with no new offenses
- [ ] Ran `msftidy` on changed module files with no new offenses _(modules only)_
- [ ] Ran `msftidy_docs` on changed documentation files with no new offenses _(documentation files only)_
- [ ] Included a corresponding documentation markdown file in `documentation/modules` _(new modules only)_
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm also tempted to add here that with the lowered barrier of AI, maybe we should be looking for contributors to include rspec tests? I know this is fraught. I know the best unit tests are made by someone who has never seen the code, but the reality is we are looking at chosing between suboptimal rspec tests and no rspec tests in many cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants