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

feat: build failure message from invalid hunks #90

Merged
merged 5 commits into from
Aug 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/github-handler/comment-handler/invalid-hunk-handler/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import {Hunk} from '../../../types';

function hunkErrorMessage(hunk: Hunk): string {
return ` * lines ${hunk.oldStart}-${hunk.oldEnd}`;
}

function fileErrorMessage(filename: string, hunks: Hunk[]): string {
return `* ${filename}\n` + hunks.map(hunkErrorMessage).join('\n');
}

/**
* Build an error message based on invalid hunks.
* Returns an empty string if the provided hunks are empty.
* @param invalidHunks a map of filename to hunks that are not suggestable
*/
export function buildErrorMessage(invalidHunks: Map<string, Hunk[]>): string {
if (invalidHunks.size === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why return an empty string? if there is no error message to be made there should be no message.
You can make the return type : string | null to return null

Copy link
Contributor Author

@chingor13 chingor13 Aug 21, 2020

Choose a reason for hiding this comment

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

I'm not sure how this was going to be used. I'd assume we actually want this check outside this and handle that case in the caller.

I chose to return an empty string to be more confident about types in what's being returned. I could note in the description that we will return empty string for an empty input but 🤷. If you were going to check if the result of this was empty or null, you might as well check that the input is empty before calling this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup that works

return '';
}
return (
'Some suggestions could not be made:\n' +
Array.from(invalidHunks, ([filename, hunks]) =>
fileErrorMessage(filename, hunks)
).join('\n')
);
}
65 changes: 65 additions & 0 deletions test/invalid-hunks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import {expect} from 'chai';
import {describe, it, before} from 'mocha';
import {setup} from './util';
import {buildErrorMessage} from '../src/github-handler/comment-handler/invalid-hunk-handler';

before(() => {
setup();
});

describe('buildErrorMessage', () => {
it('should handle an empty list of failures', () => {
const invalidHunks = new Map();
const expectedMessage = '';

const errorMessage = buildErrorMessage(invalidHunks);
expect(errorMessage).to.be.equal(expectedMessage);
});

it('should handle multiple file entries', () => {
const invalidHunks = new Map();
invalidHunks.set('foo.txt', [
{oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2},
]);
invalidHunks.set('bar.txt', [
{oldStart: 3, oldEnd: 4, newStart: 3, newEnd: 4},
]);
const expectedMessage = `Some suggestions could not be made:
* foo.txt
* lines 1-2
* bar.txt
* lines 3-4`;

const errorMessage = buildErrorMessage(invalidHunks);
expect(errorMessage).to.be.equal(expectedMessage);
});

it('should handle multiple entries for a file', () => {
const invalidHunks = new Map();
invalidHunks.set('foo.txt', [
{oldStart: 1, oldEnd: 2, newStart: 1, newEnd: 2},
{oldStart: 3, oldEnd: 4, newStart: 3, newEnd: 4},
]);
const expectedMessage = `Some suggestions could not be made:
* foo.txt
* lines 1-2
* lines 3-4`;

const errorMessage = buildErrorMessage(invalidHunks);
expect(errorMessage).to.be.equal(expectedMessage);
});
});