Skip to content

/allow: gracefully handle absence of GitHub user name again #412

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

Merged
webstech merged 1 commit into from
Jan 9, 2021
Merged

/allow: gracefully handle absence of GitHub user name again #412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 5 deletions lib/github-glue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,15 +464,11 @@ export class GitHubGlue {
username: login,
});

if (!response.data.name) {
Copy link
Copy Markdown
Contributor

@webstech webstech Jan 8, 2021

Choose a reason for hiding this comment

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

Sorry. Does the return need to be name: response.data.name || "",? The name changed from string to string|null. Returning an empty string vs changing the interface and handling other upstream changes (ie lib/gitgitgadget.ts:284:44) seems simpler.

Copy link
Copy Markdown
Contributor

@webstech webstech Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm. The upstream error is on using userInfo.name. That should probably be changed to pr.author if null user names are allowed. Also means the interface could be changed to allow null.

Copy link
Copy Markdown
Member Author

@dscho dscho Jan 8, 2021

Choose a reason for hiding this comment

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

Null user names are rejected because a name is required on whose behalf GitGitGadget sends the mails (or at least the cover letter). However, we do support /allowing before a name is set (but /submiting will still fail, with the error message "Error: Could not determine full name of [name]", see here).

Does the return need to be name: response.data.name || "",?

Good point, I made it so.

Copy link
Copy Markdown
Contributor

@webstech webstech Jan 9, 2021

Choose a reason for hiding this comment

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

Hmmm. The upstream error is on using userInfo.name. That should probably be changed to pr.author if null user names are allowed. Also means the interface could be changed to allow null.

Just noting for future clarity this comment is incorrect since the two values are not equivalent.

throw new Error(`Unable to get name for ${login} - ${
response.data.toString()}`);
}

return {
email: response.data.email,
login: response.data.login,
name: response.data.name,
name: response.data.name || "",
type: response.data.type,
};
}
Expand Down
4 changes: 1 addition & 3 deletions tests/github-glue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,9 +587,7 @@ test("test missing values in response using small schema", async () => {

(sampleUser as IPrivateUser).name = null;
// if (!response.data.name) {
await expect(github.getGitHubUserInfo(owner)).rejects.toThrow(
/Unable to get name/
);
await expect(github.getGitHubUserInfo(owner)).toBeTruthy();

// if (!response.data.name) {
await expect(github.getGitHubUserName(owner)).rejects.toThrow(
Expand Down