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.

Sign up for GitHub

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

Jump to bottom

Add labels in a more principled way. #90

Merged
merged 4 commits into from
Nov 14, 2024
Merged

Add labels in a more principled way. #90

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2631047
Add labels in a more principled way.
csilvers Nov 12, 2024
10570dc
rebuild dist file
csilvers Nov 12, 2024
24a71c0
share a type
csilvers Nov 12, 2024
2b22ed6
update setup-files with documentation
csilvers Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions dist/index.js
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 10 additions & 4 deletions src/__test__/main.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,17 +314,23 @@ describe('test that changes to verified commits dont notify people', () => {

describe('test that makeCommitComment makes well formatted strings', () => {
it('should format the commit comment nicely', async () => {
const peopleToFiles = {
'@yipstanley': ['src/runOnPush.js', '.github/workflows/build.yml'],
'@Khan/frontend-infra': ['src/runOnPush.js', '.geraldignore'],
const peopleToLabelToFiles = {
'@yipstanley': {
'': ['src/runOnPush.js', '.github/workflows/build.yml'],
typechanges: ['flow-typed/npm/@octokit/rest_vx.x.x.js'],
},
'@Khan/frontend-infra': {
'': ['src/runOnPush.js', '.geraldignore'],
},
};

await __makeCommitComment(peopleToFiles, 'suite5-commit1');
await __makeCommitComment(peopleToLabelToFiles, 'suite5-commit1');

expect(await getComment('suite5-commit1')).toMatchInlineSnapshot(`
"Notify of Push Without Pull Request

@yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
@yipstanley for changes to \`flow-typed/npm/@octokit/rest_vx.x.x.js\` (typechanges)
@Khan/frontend-infra for changes to \`src/runOnPush.js\`, \`.geraldignore\`
"
`);
Expand Down
126 changes: 74 additions & 52 deletions src/__test__/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,30 +63,30 @@ describe('maybe add', () => {
let pattern = /test/;
let diffs = {testFile: 'a diff with the word test'};
const name = 'testName';
const bin = {testName: []};
const bin = {testName: {'': []}};
const filesChanged = ['testFile'];

__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);

expect(bin).toEqual({testName: ['testFile']});
expect(bin).toEqual({testName: {'': ['testFile']}});

// it shouldn't re-add 'testFile' or another testName key
__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);

expect(bin).toEqual({testName: ['testFile']});
expect(bin).toEqual({testName: {'': ['testFile']}});

pattern = /nonexistent/;

__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);

expect(bin).toEqual({testName: ['testFile']});
expect(bin).toEqual({testName: {'': ['testFile']}});

pattern = /existent/;
diffs = {otherFile: 'the word existent exists in this diff'};

__maybeAddIfMatch(pattern, name, diffs, bin, filesChanged);
__maybeAddIfMatch(pattern, name, '', diffs, bin, filesChanged);

expect(bin).toEqual({testName: ['testFile']});
expect(bin).toEqual({testName: {'': ['testFile']}});
});
});

Expand Down Expand Up @@ -150,12 +150,20 @@ describe('push or set to bin', () => {
const bin = {};
const username = 'testName';
let files = ['file1', 'file2'];
__pushOrSetToBin(bin, username, files);
expect(bin).toEqual({testName: files});
__pushOrSetToBin(bin, username, '', files);
expect(bin).toEqual({testName: {'': files}});

files = ['file2', 'file3', 'file4'];
__pushOrSetToBin(bin, username, files);
expect(bin).toEqual({testName: ['file1', 'file2', 'file3', 'file4']});
__pushOrSetToBin(bin, username, '', files);
expect(bin).toEqual({testName: {'': ['file1', 'file2', 'file3', 'file4']}});

__pushOrSetToBin(bin, username, 'mylabel', files);
expect(bin).toEqual({
testName: {
'': ['file1', 'file2', 'file3', 'file4'],
mylabel: ['file2', 'file3', 'file4'],
},
});
});
});

Expand Down Expand Up @@ -191,14 +199,18 @@ mylabel: src/*Push.js @owner`,
expect(
await getNotified(filesChanged, fileDiffs, {}, 'testAuthor', 'pull_request'),
).toEqual({
'@yipstanley': ['src/execCmd.js', 'src/runOnPush.js'],
'@githubUser': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
'@testPerson': ['.github/workflows/build.yml'],
'@yipstanley': {'': ['src/execCmd.js', 'src/runOnPush.js']},
'@githubUser': {
'': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
},
'@testPerson': {'': ['.github/workflows/build.yml']},
});

expect(await getNotified(filesChanged, fileDiffs, {}, 'testAuthor', 'push')).toEqual({
'@owner': ['src/execCmd.js', 'src/runOnPush.js'],
'@owner (mylabel)': ['src/runOnPush.js'],
'@owner': {
'': ['src/execCmd.js', 'src/runOnPush.js'],
mylabel: ['src/runOnPush.js'],
},
});
});

Expand Down Expand Up @@ -234,15 +246,17 @@ mylabel: src/*Push.js @owner`,
notifiedFile,
),
).toEqual({
'@yipstanley': ['src/execCmd.js', 'src/runOnPush.js'],
'@githubUser': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
'@testPerson': ['.github/workflows/build.yml'],
'@yipstanley': {'': ['src/execCmd.js', 'src/runOnPush.js']},
'@githubUser': {
'': ['.github/workflows/build.yml', 'src/execCmd.js', 'src/runOnPush.js'],
},
'@testPerson': {'': ['.github/workflows/build.yml']},
});

expect(
await getNotified(filesChanged, fileDiffs, {}, '__testUser', 'push', notifiedFile),
).toEqual({
'@owner': ['src/execCmd.js', 'src/runOnPush.js'],
'@owner': {'': ['src/execCmd.js', 'src/runOnPush.js']},
});
});
});
Expand Down Expand Up @@ -272,11 +286,11 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
'@testPerson': ['.github/workflows/build.yml'],
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
'@testPerson': {'': ['.github/workflows/build.yml']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});

Expand Down Expand Up @@ -304,10 +318,10 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});

Expand Down Expand Up @@ -335,11 +349,11 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@githubUser': ['src/execCmd.js', 'src/runOnPush.js'],
'@testPerson': ['.github/workflows/build.yml'],
'@githubUser': {'': ['src/execCmd.js', 'src/runOnPush.js']},
'@testPerson': {'': ['.github/workflows/build.yml']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});

Expand All @@ -365,10 +379,10 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@testPerson': ['.github/workflows/build.yml'],
'@testPerson': {'': ['.github/workflows/build.yml']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});

Expand All @@ -394,10 +408,10 @@ describe('get reviewers', () => {
'yipstanley',
);
expect(reviewers).toEqual({
'@testPerson': ['.github/workflows/build.yml'],
'@testPerson': {'': ['.github/workflows/build.yml']},
});
expect(requiredReviewers).toEqual({
'@githubUser': ['.github/workflows/build.yml'],
'@githubUser': {'': ['.github/workflows/build.yml']},
});
});
});
Expand Down Expand Up @@ -586,22 +600,28 @@ describe('test that ignore files are parsed correctly', () => {

describe('test that makeCommentBody makes a nicely-formatted string', () => {
it('should format the Gerald pull request comment correctly', async () => {
const peopleToFiles = {
'@yipstanley': [
'src/runOnPush.js',
'.github/workflows/build.yml',
'flow-typed/npm/@octokit/rest_vx.x.x.js',
],
'@Khan/frontend-infra': ['src/runOnPush.js', '.geraldignore'],
const peopleToLabelToFiles = {
'@yipstanley': {
'': ['src/runOnPush.js', '.github/workflows/build.yml'],
typechanges: ['flow-typed/npm/@octokit/rest_vx.x.x.js'],
},
'@Khan/frontend-infra': {
'': ['src/runOnPush.js', '.geraldignore'],
},
};

const result = await makeCommentBody({peopleToFiles, header: 'Reviewers', tagPerson: true});
const result = await makeCommentBody({
peopleToLabelToFiles,
header: 'Reviewers',
tagPerson: true,
});

expect(result).toMatchInlineSnapshot(`
"<details>
<summary><b>Reviewers</b></summary>

* @yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`, \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\`
* @yipstanley for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
* @yipstanley for changes to \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\` (typechanges)
* @Khan/frontend-infra for changes to \`src/runOnPush.js\`, \`.geraldignore\`
</details>

Expand All @@ -610,17 +630,18 @@ describe('test that makeCommentBody makes a nicely-formatted string', () => {
});

it('should comment out reviewers', async () => {
const peopleToFiles = {
'@yipstanley': [
'src/runOnPush.js',
'.github/workflows/build.yml',
'flow-typed/npm/@octokit/rest_vx.x.x.js',
],
'@Khan/frontend-infra': ['src/runOnPush.js', '.geraldignore'],
const peopleToLabelToFiles = {
'@yipstanley': {
'': ['src/runOnPush.js', '.github/workflows/build.yml'],
typechanges: ['flow-typed/npm/@octokit/rest_vx.x.x.js'],
},
'@Khan/frontend-infra': {
'': ['src/runOnPush.js', '.geraldignore'],
},
};

const result = await makeCommentBody({
peopleToFiles,
peopleToLabelToFiles,
header: 'Reviewers',
tagPerson: false,
});
Expand All @@ -629,7 +650,8 @@ describe('test that makeCommentBody makes a nicely-formatted string', () => {
"<details>
<summary><b>Reviewers</b></summary>

* \`@yipstanley\` for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`, \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\`
* \`@yipstanley\` for changes to \`src/runOnPush.js\`, \`.github/workflows/build.yml\`
* \`@yipstanley\` for changes to \`flow-typed/npm/%40@octokit/rest_vx.x.x.js\` (typechanges)
* \`@Khan/frontend-infra\` for changes to \`src/runOnPush.js\`, \`.geraldignore\`
</details>

Expand Down
14 changes: 8 additions & 6 deletions src/runOnPullRequest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import {
MATCH_COMMENT_HEADER_REGEX,
} from './constants';

type NameToLabelToFiles = {[name: string]: {[label: string]: string[], ...}, ...};
Copy link
Member

@lillialexis lillialexis Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before, if there were multiple matching rules, I think it would do:

{
  lilli: [fileFromR1, fileFromR2, fileFromR2]
  ...
}

Then in the comment it would do:

lilli because fileFromR1, fileFromR2, fileFromR2

But now if there are multiple matching rules, I think I would want:

lilli (rule1): fileFromR1
lilli (rule2): fileFromR2, fileFromR2

I'm not clear if this structure gets us to that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does! You can see in some of the test output.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it does, but the syntax is starting to twist my head...

Copy link
Member Author

Choose a reason for hiding this comment

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

If js allowed non-string map keys, I could do a map from (person, label) to [files], but it doesn't, so this is the best I could do...


/**
* @desc Helper function to update, delete, or create a comment
* @param comment - existing Github comment to update/delete or undefined
Expand All @@ -30,23 +32,23 @@ import {
*/
const updatePullRequestComment = async (
comment: ?Octokit$IssuesListCommentsResponseItem,
notifyees: {[string]: Array<string>, ...},
reviewers: {[string]: Array<string>, ...},
requiredReviewers: {[string]: Array<string>, ...},
notifyees: NameToLabelToFiles,
reviewers: NameToLabelToFiles,
requiredReviewers: NameToLabelToFiles,
) => {
let body: string = GERALD_COMMENT_HEADER;
body += makeCommentBody({
peopleToFiles: notifyees,
peopleToLabelToFiles: notifyees,
header: GERALD_COMMENT_NOTIFIED_HEADER,
tagPerson: true,
});
body += makeCommentBody({
peopleToFiles: reviewers,
peopleToLabelToFiles: reviewers,
header: GERALD_COMMENT_REVIEWERS_HEADER,
tagPerson: false,
});
body += makeCommentBody({
peopleToFiles: requiredReviewers,
peopleToLabelToFiles: requiredReviewers,
header: GERALD_COMMENT_REQ_REVIEWERS_HEADER,
tagPerson: false,
});
Expand Down
19 changes: 11 additions & 8 deletions src/runOnPush.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,19 @@ import {execCmd} from './execCmd';
import {ownerAndRepo, extraPermGithub, type Context} from './setup';
import {PUSH, GERALD_COMMIT_COMMENT_HEADER} from './constants';

const makeCommitComment = async (
peopleToFiles: {[string]: Array<string>, ...},
commitSHA: string,
) => {
const names: string[] = Object.keys(peopleToFiles);
if (peopleToFiles && names.length) {
type NameToLabelToFiles = {[name: string]: {[label: string]: string[], ...}, ...};
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this somewhere shared so as to not dupe?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you want. I like having it in each place so it's easier to find when reading the file; flow won't let them get out of sync. But I know some people feel very militant about DRY!


const makeCommitComment = async (peopleToLabelToFiles: NameToLabelToFiles, commitSHA: string) => {
const names: string[] = Object.keys(peopleToLabelToFiles);
if (peopleToLabelToFiles && names.length) {
let body: string = GERALD_COMMIT_COMMENT_HEADER;
names.forEach((person: string) => {
const files = peopleToFiles[person];
body += `${person} for changes to \`${files.join('`, `')}\`\n`;
const labels: string[] = Object.keys(peopleToLabelToFiles[person]);
labels.forEach((label: string) => {
const files = peopleToLabelToFiles[person][label];
const labelText = label ? ` (${label})` : '';
body += `${person} for changes to \`${files.join('`, `')}\`${labelText}\n`;
});
});

await extraPermGithub.repos.createCommitComment({
Expand Down
Loading