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

Create PR revert template #16446

Merged
merged 1 commit into from May 7, 2024
Merged

Create PR revert template #16446

merged 1 commit into from May 7, 2024

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Apr 24, 2024

Link to issue number:

None

Summary of the issue:

Revert PRs are janitorial actions which do not make sense to follow the PR template.
Typically these are made adhoc using the GitHub UX.
When performing a revert, it is important to note issues being opened and closed by the reversion.
It is also vital to note the PR being reverted.
Adding a short reason for reversion will help people understand a summary of what is going on.

Description of user facing changes

None

Description of development approach

Created a new template to be copy-pasted when performing a revert

Testing strategy:

N/A

Known issues with pull request:

GitHub has a suggested UX for multiple PR templates: https://github.blog/2018-01-25-multiple-issue-and-pull-request-templates/
The GitHub UX for having multiple PR templates is less than ideal.
It's like landing at https://github.com/nvaccess/nvda/issues/new rather than https://github.com/nvaccess/nvda/issues/new/choose.
I figure this process may annoy some contributors.

Additionally, when performing a revert through GitHubs UX, the PR template is automatically overriden anyway, meaning the UX for multiple PR templates is bypassed anyway.
I've opened a feature request here to improve this design: https://github.com/orgs/community/discussions/121757

Code Review Checklist:

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

@seanbudd seanbudd requested a review from a team as a code owner April 24, 2024 04:00
@CyrilleB79
Copy link
Collaborator

IMO, the expectation from NV Access should be clarified when reverting, maybe in the "Reason for revert" section?

E.g.:
"We have reverted because the feature is buggy and we are in beta stage" is correct.
But it may not be enough for a dev who want to tackle the issue again. The dev want to have such additional information:

  • the feature may be accepted if the issue #XYZ is fixed in the future?
  • some design decision need to be made before a new implementation is proposed
  • NV Access has changed their mind and think this feature should rather belong to an add-on, due to XYZ.

This information may be provided in the revert PR or in the issue being reopened (if any), but in any case, it should be made available at the same time the revert occurs.

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 30, 2024
@seanbudd
Copy link
Member Author

seanbudd commented May 1, 2024

I've updated this PR significantly to avoid GitHub's poor UX for multiple PRs.

@CyrilleB79 - do you think anything is missing from the new template?

@AppVeyorBot
Copy link

See test results for failed build of commit dcaa64e51e

@seanbudd seanbudd added this to the 2024.3 milestone May 1, 2024
@seanbudd seanbudd modified the milestones: 2024.3, 2024.2 May 7, 2024
@seanbudd seanbudd merged commit 1febec3 into nvaccess:master May 7, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. queued for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants