Skip to content

Commit

Permalink
docs(pie-monorepo): DSW-000 update the pull request template (#2100)
Browse files Browse the repository at this point in the history
* docs(pie-monorepo): DSW-000 update the pull request template

* add table for links

* grammar

* add testing guide

* update danger

* dangerjs update

* add extra instruction about n/a items to authors part

* add back comments
  • Loading branch information
jamieomaguire authored Nov 29, 2024
1 parent 59743d9 commit 88fcb8f
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 21 deletions.
41 changes: 31 additions & 10 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,40 @@ Please click the `Preview` tab and select a different PR template if your change
## Describe your changes (can list changeset entries if preferable)


## Author Checklist (complete before requesting a review)
- [ ] I have performed a self-review of my code
- [ ] I have added thorough tests where applicable (unit / component / visual)
- [ ] I have reviewed the `PIE Storybook`/`PIE Docs` PR preview
- [ ] I have reviewed visual test updates properly before approving
## Author Checklist (complete before requesting a review, do not delete any)
- [ ] I have performed a self-review of my code.
- [ ] I have added thorough tests where applicable (unit / component / visual).
- [ ] I have reviewed the `PIE Storybook`/`PIE Docs` PR preview.
- [ ] I have reviewed visual test updates properly before approving.
- [ ] If changes will affect consumers of the package, I have created a changeset entry.
- [ ] If a changeset file has been created, I have used the `/snapit` functionality to test my changes in a consuming application
- [ ] If a changeset file has been created, I have tested these changes in [PIE Aperture](https://github.com/justeattakeaway/pie-aperture/) using the `/test-aperture` command.

## Not-applicable Checklist items
Please move any Author checklist items that do not apply to this pull request here.

---

## Testing
[How do I test my changes?](https://github.com/justeattakeaway/pie/wiki/PIE-Aperture)

| Task | Link |
|------------------------|----------------------------------|
| Aperture PR | [🔗](#) |
| NextJS 14 deployment | [🔗](#) |
| Nuxt 3 deployment | [🔗](#) |
| Vanilla deployment | [🔗](#) |

## Reviewer checklists (complete before approving)
Mark items as `[-] N/A` if not applicable.

### Reviewer 1
- [ ] I have reviewed the `PIE Storybook`/`PIE Docs` PR preview
- [ ] If there are visual test updates, I have reviewed them
- [ ] I have reviewed the `PIE Storybook`/`PIE Docs` PR preview.
- [ ] I have verified that all acceptance criteria for this ticket have been completed.
- [ ] I have reviewed the Aperture changes (if added)
- [ ] If there are visual test updates, I have reviewed them.

### Reviewer 2
- [ ] I have reviewed the `PIE Storybook`/`PIE Docs` PR preview
- [ ] If there are visual test updates, I have reviewed them
- [ ] I have reviewed the `PIE Storybook`/`PIE Docs` PR preview.
- [ ] I have verified that all acceptance criteria for this ticket have been completed.
- [ ] I have reviewed the Aperture changes (if added)
- [ ] If there are visual test updates, I have reviewed them.
10 changes: 5 additions & 5 deletions apps/pie-storybook/data/tag-variants-to-statuses-map.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { type TagVariantToStatusMap } from '../interfaces/tag-variant-to-status-map';

export const tagVariantToStatusMap: TagVariantToStatusMap = {
alpha: 'yellow',
beta: 'yellow',
deprecated: 'red',
removed: 'red',
stable: 'green',
alpha: 'brand-05',
beta: 'brand-05',
deprecated: 'error',
removed: 'error',
stable: 'success',
};
50 changes: 44 additions & 6 deletions dangerfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ const validChangesetCategories = ['Added', 'Changed', 'Removed', 'Fixed'];
const isDependabotPR = pr.user.login === 'dependabot[bot]';

// Check for correct Changeset formatting
danger.git.created_files.filter((filepath) => filepath.includes('.changeset/') && !filepath.includes('.changeset/pre.json'))
danger.git.created_files
.filter((filepath) => filepath.includes('.changeset/') && !filepath.includes('.changeset/pre.json'))
.forEach((filepath) => {
// get the git diff for the changeset file
const changesetDiff = danger.git.diffForFile(filepath);
changesetDiff.then((result) => {
const diffString = result.diff;
Expand All @@ -27,7 +27,7 @@ danger.git.created_files.filter((filepath) => filepath.includes('.changeset/') &
// Check that categories are followed are in the following format `[Category] - {Description}`
const changesetLineFormatRegex = /\[\w+\] - [\S].+/g;
const validChangesetEntries = diffString.match(changesetLineFormatRegex);
const numberOfValidChangesetEntries = (validChangesetEntries === null ? 0 : validChangesetEntries.length);
const numberOfValidChangesetEntries = validChangesetEntries ? validChangesetEntries.length : 0;
if (numberOfCategories !== numberOfValidChangesetEntries) {
fail(`:memo: Your changeset entries should be in the format: \`[Category] - {Description}\`. One or more of your entries does not follow this format. Filepath: \`${filepath}`);
}
Expand All @@ -37,7 +37,45 @@ danger.git.created_files.filter((filepath) => filepath.includes('.changeset/') &
});
});

// Check for empty PR Description checkboxes - but not for automated version PRs
if (pr.body.includes('- [ ]') && !isDependabotPR) {
fail('You currently have an unchecked checklist item in your PR description.\n\nPlease confirm this check has been carried out – if it\'s not relevant to your PR, delete this line from the PR checklist.');
// Allow unchecked checkboxes only in "Not-applicable Checklist items"
if (!isDependabotPR) {
const { body } = pr;

const uncheckedCheckboxRegex = /- \[ \]/g;

// Split the body into sections
const sections = body.split(/## /);
const notApplicableSection = sections.find((section) => section.startsWith('Not-applicable Checklist items'));

// Check for unchecked checkboxes outside the "Not-applicable Checklist items" section
const uncheckedOutsideNotApplicableSection = sections.some((section) => {
if (section !== notApplicableSection) {
return uncheckedCheckboxRegex.test(section);
}
return false;
});

if (uncheckedOutsideNotApplicableSection) {
fail('You have unchecked checklist items outside the "Not-applicable Checklist items" section.\n\nPlease ensure all unchecked checkboxes are moved to the appropriate section.');
}

// Match sections for Reviewer 1 and Reviewer 2
const reviewer1Section = body.match(/### Reviewer 1.*?(?=###|$)/s);
const reviewer2Section = body.match(/### Reviewer 2.*?(?=###|$)/s);

// Helper function to check a reviewer's section
const checkReviewerSection = (section, reviewerName) => {
if (section) {
const uncheckedReviewerCheckboxes = section.match(uncheckedCheckboxRegex);
if (uncheckedReviewerCheckboxes) {
fail(`You have unchecked checklist items in ${reviewerName}'s section.\n\nPlease ensure all items are addressed before approval.`);
}
}
};

// Check Reviewer 1
checkReviewerSection(reviewer1Section ? reviewer1Section[0] : null, 'Reviewer 1');

// Check Reviewer 2
checkReviewerSection(reviewer2Section ? reviewer2Section[0] : null, 'Reviewer 2');
}

0 comments on commit 88fcb8f

Please sign in to comment.