-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Prepare for a Typed Octokat #1937
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some notes on the proposed changes
app/src/lib/api.ts
Outdated
@@ -76,10 +76,9 @@ export interface IAPICommit { | |||
export interface IAPIUser { | |||
readonly id: number | |||
readonly url: string | |||
readonly type: 'user' | 'org' |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -554,7 +553,7 @@ export async function fetchUser(endpoint: string, token: string): Promise<Accoun | |||
emails = [ ] | |||
} | |||
|
|||
return new Account(user.login, endpoint, token, emails, user.avatarUrl, user.id, user.name) | |||
return new Account(user.login, endpoint, token, emails, user.avatar_url, user.id, user.name) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -31,7 +31,7 @@ export class UserAutocompletionProvider implements IAutocompletionProvider<IUser | |||
|
|||
public async getAutocompletionItems(text: string): Promise<ReadonlyArray<IUserHit>> { | |||
const users = await this.gitHubUserStore.getMentionableUsersMatching(this.repository, text) | |||
return users.map(u => ({ username: u.login, name: u.name })) | |||
return users.map(u => ({ username: u.login, name: u.name || u.login })) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @philschatz! It's awesome you're looking into providing typings!
I'm a little concerned about the knock-on effects of making name
optional everywhere. I imagine we should split it into two types so we're clear about where we can rely on name
and where we can't.
Thanks for the feedback! I split the
As a result, the User autocomplete no longer tries to show the user name (it did not work before anyway). Aside: I tend to be extra-verbose so it is easier to replace the names later but can rename them to something shorter like |
Thanks @philschatz! We're getting close:
It seems to be working for me currently, or maybe I'm misunderstanding? |
Thanks for all your work here @philschatz! I'm personally really excited to see a strongly typed Octokat! With #2080 moving us over to issuing API requests manually I'm going to close this PR. |
Many responses from GitHub's API yield JSON that describes a User but are missing the
name:
field (see below for a partial list).This Pull Request changes the API
interface
to match that expectation.This work is part of philschatz/octokat.js#176 (add TypeScript Definitions) and refs #1522 and #1281
To verify that these are the only changes needed, I plugged the
octokat/index.d.ts
file into Desktop and ran the CI tests. Here is a branch of that work: philschatz/desktop@master...philschatz:octokat-typescript-exampleGitHub API Routes and User Objects
The following routes yield a User with a
name:
field in the Response JSON:The following routes (39) yield a User that does not contain the
name:
field in the Response JSON:[Click me] to see all the routes...