Skip to content

Add development approach to PR template#13730

Merged
seanbudd merged 3 commits into
masterfrom
addDevApproach
Jun 17, 2022
Merged

Add development approach to PR template#13730
seanbudd merged 3 commits into
masterfrom
addDevApproach

Conversation

@seanbudd

Copy link
Copy Markdown
Member

Link to issue number:

None

Summary of the issue:

In order to aid reviewing complex pull requests, having a description of the development approach is helpful.
Often this is implicitly included in "Description of how this pull request fixes the issue", however this is also used as a high level summary of the user facing changes. A separation of concerns is needed.

Description of how this pull request fixes the issue:

Adds this information to the PR template and explanation.

Testing strategy:

Known issues with pull request:

n/a

Change log entries:

n/a

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English

@seanbudd seanbudd requested a review from a team as a code owner May 24, 2022 01:18
@seanbudd seanbudd requested review from feerrenrut and michaelDCurran and removed request for michaelDCurran May 24, 2022 01:18
@AppVeyorBot

Copy link
Copy Markdown

See test results for failed build of commit 11ee94ef80

@feerrenrut feerrenrut left a comment

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.

It might be worth clarifying that Description of how this pull request fixes the issue: is now intended to be user / use-case / ux focused.

Originally summary of the issue was meant to be use-case / ux need focused, and Description of how this pull request fixes the issue was meant to be development focused. Note the suggestion for links to external information.

I'm happy to have 3 sections:

  • This is a summary of use-case / problem statement.
  • This is the UX I've implemented to fix it.
  • This is a technical description of how I addressed it.

This helps others to have the same background as you and learn from this work.

### Description of development approach
Provide a description of the development approach followed.

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.

The way this is written currently sounds like it's asking for an answer like "test driven development", whereas I think you want a technical overview, answering questions like:

  • How does data flow?
  • What components are involved and how do they relate to each other?
  • Example data to be processed?

Providing an example could help.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've changed the terminology from "development approach" to "technical changes".

I've also added a simple example.

@seanbudd seanbudd removed the request for review from michaelDCurran June 15, 2022 06:29
@seanbudd seanbudd requested a review from feerrenrut June 15, 2022 06:36
@seanbudd seanbudd merged commit 85b039b into master Jun 17, 2022
@seanbudd seanbudd deleted the addDevApproach branch June 17, 2022 00:34
@nvaccessAuto nvaccessAuto added this to the 2022.3 milestone Jun 17, 2022
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.

4 participants