From f219636a56144ee1c38c7d7175f919aea6352e48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Mon, 7 May 2018 17:28:47 -0700 Subject: [PATCH] Create "REPOSITORY" nodes in GitHub plugin graph (#229) This commit creates a new node type in the GitHub graph: the REPOSITORY node. The REPOSITORY node has the following payload properties: - url (string) - name (string) - owner (string) Things this commit does: - Add new node type and payload type (RepositoryNodePayload) - Update parser to instantiate the new node type - Update api.js to have Repository wrap the new node type (thus Repository is a GitHub entity) - Update snapshots - Update users of GitHub node types to ensure they are exhaustive Things that will come in a followon commit: - Add CONTAINS edges from the repository to all its PRs and Issues - Update the Repository porcelain to use those edges, rather than scanning the graph for every possible Issue/PR (eventually those might belong to other Repositories) - Create a GitHubGraph abstraction in the porcelain, which makes it easy to find all of the Repositories in a graph Note that retrieving the repository owner technically involved fetching the whole owner representation (as a GitHub user). I could have chosen to add that user to the graph, with a "OWNS" edge pointing to the repository. For simplicity's sake, I've declined to do that, and instead just parse the owner's name directly. Test plan: Added tests to verify that the Repository porcelain entity has the right properties. Combined with the snapshot tests, that should be sufficient. --- .../githubPluginAdapter.test.js.snap | 15 +++ .../editor/adapters/githubPluginAdapter.js | 6 ++ .../github/__snapshots__/parser.test.js.snap | 49 ++++++++++ src/plugins/github/api.js | 93 +++++++++++-------- src/plugins/github/api.test.js | 9 +- src/plugins/github/parser.js | 13 +++ src/plugins/github/types.js | 13 +++ 7 files changed, 156 insertions(+), 42 deletions(-) diff --git a/src/plugins/artifact/editor/adapters/__snapshots__/githubPluginAdapter.test.js.snap b/src/plugins/artifact/editor/adapters/__snapshots__/githubPluginAdapter.test.js.snap index c0d1236..3cd8931 100644 --- a/src/plugins/artifact/editor/adapters/__snapshots__/githubPluginAdapter.test.js.snap +++ b/src/plugins/artifact/editor/adapters/__snapshots__/githubPluginAdapter.test.js.snap @@ -17,6 +17,21 @@ Array [ "title": "decentralion", "type": "AUTHOR", }, + Object { + "id": "https://github.com/sourcecred/example-github", + "payload": Object { + "name": "example-github", + "owner": "sourcecred", + "url": "https://github.com/sourcecred/example-github", + }, + "rendered":
+ type: + REPOSITORY + (details to be implemented) +
, + "title": "sourcecred/example-github", + "type": "REPOSITORY", + }, Object { "id": "https://github.com/sourcecred/example-github/issues/1", "payload": Object { diff --git a/src/plugins/artifact/editor/adapters/githubPluginAdapter.js b/src/plugins/artifact/editor/adapters/githubPluginAdapter.js index ec79905..1589d72 100644 --- a/src/plugins/artifact/editor/adapters/githubPluginAdapter.js +++ b/src/plugins/artifact/editor/adapters/githubPluginAdapter.js @@ -7,6 +7,7 @@ import type {Node} from "../../../../core/graph"; import type { NodePayload, NodeType, + RepositoryNodePayload, IssueNodePayload, PullRequestNodePayload, CommentNodePayload, @@ -46,6 +47,9 @@ const adapter: PluginAdapter = { return adapter.extractTitle(graph, graph.node(neighbor)); }); } + function extractRepositoryTitle(node: Node) { + return `${node.payload.owner}/${node.payload.name}`; + } function extractIssueOrPrTitle( node: Node ) { @@ -80,6 +84,8 @@ const adapter: PluginAdapter = { const anyNode: Node = node; const type: NodeType = (node.address.type: any); switch (type) { + case "REPOSITORY": + return extractRepositoryTitle(anyNode); case "ISSUE": case "PULL_REQUEST": return extractIssueOrPrTitle(anyNode); diff --git a/src/plugins/github/__snapshots__/parser.test.js.snap b/src/plugins/github/__snapshots__/parser.test.js.snap index fa308b4..3653dee 100644 --- a/src/plugins/github/__snapshots__/parser.test.js.snap +++ b/src/plugins/github/__snapshots__/parser.test.js.snap @@ -25,6 +25,13 @@ Object { "url": "https://github.com/decentralion", }, }, + "{\\"id\\":\\"https://github.com/sourcecred/example-github\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"REPOSITORY\\"}": Object { + "payload": Object { + "name": "example-github", + "owner": "sourcecred", + "url": "https://github.com/sourcecred/example-github", + }, + }, "{\\"id\\":\\"https://github.com/sourcecred/example-github/issues/1\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"ISSUE\\"}": Object { "payload": Object { "body": "This is just an example issue.", @@ -140,6 +147,13 @@ Object { "url": "https://github.com/decentralion", }, }, + "{\\"id\\":\\"https://github.com/sourcecred/example-github\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"REPOSITORY\\"}": Object { + "payload": Object { + "name": "example-github", + "owner": "sourcecred", + "url": "https://github.com/sourcecred/example-github", + }, + }, "{\\"id\\":\\"https://github.com/sourcecred/example-github/issues/6\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"ISSUE\\"}": Object { "payload": Object { "body": "This issue shall shortly have a few comments.", @@ -301,6 +315,13 @@ Object { "url": "https://github.com/decentralion", }, }, + "{\\"id\\":\\"https://github.com/sourcecred/example-github\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"REPOSITORY\\"}": Object { + "payload": Object { + "name": "example-github", + "owner": "sourcecred", + "url": "https://github.com/sourcecred/example-github", + }, + }, "{\\"id\\":\\"https://github.com/sourcecred/example-github/pull/5\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"PULL_REQUEST\\"}": Object { "payload": Object { "body": "@wchargin could you please do the following: @@ -410,6 +431,13 @@ Object { "url": "https://github.com/decentralion", }, }, + "{\\"id\\":\\"https://github.com/sourcecred/example-github\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"REPOSITORY\\"}": Object { + "payload": Object { + "name": "example-github", + "owner": "sourcecred", + "url": "https://github.com/sourcecred/example-github", + }, + }, "{\\"id\\":\\"https://github.com/sourcecred/example-github/pull/3\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"PULL_REQUEST\\"}": Object { "payload": Object { "body": "Oh look, it's a pull request.", @@ -843,6 +871,13 @@ Object { "url": "https://github.com/decentralion", }, }, + "{\\"id\\":\\"https://github.com/sourcecred/example-github\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"REPOSITORY\\"}": Object { + "payload": Object { + "name": "example-github", + "owner": "sourcecred", + "url": "https://github.com/sourcecred/example-github", + }, + }, "{\\"id\\":\\"https://github.com/sourcecred/example-github/issues/1\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"ISSUE\\"}": Object { "payload": Object { "body": "This is just an example issue.", @@ -1191,6 +1226,13 @@ Object { "url": "https://github.com/decentralion", }, }, + "{\\"id\\":\\"https://github.com/sourcecred/example-github\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"REPOSITORY\\"}": Object { + "payload": Object { + "name": "example-github", + "owner": "sourcecred", + "url": "https://github.com/sourcecred/example-github", + }, + }, "{\\"id\\":\\"https://github.com/sourcecred/example-github/issues/2\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"ISSUE\\"}": Object { "payload": Object { "body": "This issue references another issue, namely #1", @@ -2016,6 +2058,13 @@ Object { "url": "https://github.com/decentralion", }, }, + "{\\"id\\":\\"https://github.com/sourcecred/example-github\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"REPOSITORY\\"}": Object { + "payload": Object { + "name": "example-github", + "owner": "sourcecred", + "url": "https://github.com/sourcecred/example-github", + }, + }, "{\\"id\\":\\"https://github.com/sourcecred/example-github/issues/1\\",\\"pluginName\\":\\"sourcecred/github-beta\\",\\"type\\":\\"ISSUE\\"}": Object { "payload": Object { "body": "This is just an example issue.", diff --git a/src/plugins/github/api.js b/src/plugins/github/api.js index 95c5941..428f380 100644 --- a/src/plugins/github/api.js +++ b/src/plugins/github/api.js @@ -18,6 +18,7 @@ import type { PullRequestReviewCommentNodePayload, PullRequestReviewNodePayload, PullRequestReviewState, + RepositoryNodePayload, } from "./types"; import { @@ -36,6 +37,7 @@ import { import {COMMIT_NODE_TYPE} from "../git/types"; export type Entity = + | Repository | Issue | PullRequest | Comment @@ -51,47 +53,6 @@ function assertEntityType(e: Entity, t: NodeType) { } } -export class Repository { - graph: Graph; - - constructor(graph: Graph) { - this.graph = graph; - } - - issueOrPRByNumber(number: number): ?(Issue | PullRequest) { - let result: Issue | PullRequest; - this.graph.nodes({type: ISSUE_NODE_TYPE}).forEach((n) => { - if (n.payload.number === number) { - result = new Issue(this.graph, n.address); - } - }); - this.graph.nodes({type: PULL_REQUEST_NODE_TYPE}).forEach((n) => { - if (n.payload.number === number) { - result = new PullRequest(this.graph, n.address); - } - }); - return result; - } - - issues(): Issue[] { - return this.graph - .nodes({type: ISSUE_NODE_TYPE}) - .map((n) => new Issue(this.graph, n.address)); - } - - pullRequests(): PullRequest[] { - return this.graph - .nodes({type: PULL_REQUEST_NODE_TYPE}) - .map((n) => new PullRequest(this.graph, n.address)); - } - - authors(): Author[] { - return this.graph - .nodes({type: AUTHOR_NODE_TYPE}) - .map((n) => new Author(this.graph, n.address)); - } -} - class GithubEntity { graph: Graph; nodeAddress: Address; @@ -118,6 +79,53 @@ class GithubEntity { } } +export class Repository extends GithubEntity { + // TODO: Now that the Repository is a node in the graph, re-write methods + // that find issues and PRs to find neighbors of the repository rather than + // any matching nodes in the graph. Then, behavior will be correct in the + // case where we have multiple repositories in the same graph. + issueOrPRByNumber(number: number): ?(Issue | PullRequest) { + let result: Issue | PullRequest; + this.graph.nodes({type: ISSUE_NODE_TYPE}).forEach((n) => { + if (n.payload.number === number) { + result = new Issue(this.graph, n.address); + } + }); + this.graph.nodes({type: PULL_REQUEST_NODE_TYPE}).forEach((n) => { + if (n.payload.number === number) { + result = new PullRequest(this.graph, n.address); + } + }); + return result; + } + + owner(): string { + return this.node().payload.owner; + } + + name(): string { + return this.node().payload.name; + } + + issues(): Issue[] { + return this.graph + .nodes({type: ISSUE_NODE_TYPE}) + .map((n) => new Issue(this.graph, n.address)); + } + + pullRequests(): PullRequest[] { + return this.graph + .nodes({type: PULL_REQUEST_NODE_TYPE}) + .map((n) => new PullRequest(this.graph, n.address)); + } + + authors(): Author[] { + return this.graph + .nodes({type: AUTHOR_NODE_TYPE}) + .map((n) => new Author(this.graph, n.address)); + } +} + class Post< T: | IssueNodePayload @@ -167,6 +175,9 @@ class Post< case "PULL_REQUEST_REVIEW_COMMENT": result.push(new PullRequestReviewComment(this.graph, neighbor)); break; + case "REPOSITORY": + result.push(new Repository(this.graph, neighbor)); + break; default: // eslint-disable-next-line no-unused-expressions (type: empty); diff --git a/src/plugins/github/api.test.js b/src/plugins/github/api.test.js index c7bc348..5b68350 100644 --- a/src/plugins/github/api.test.js +++ b/src/plugins/github/api.test.js @@ -13,7 +13,9 @@ import { } from "./types"; describe("GitHub porcelain API", () => { const graph = parse(exampleRepoData); - const repo = new Repository(graph); + // TODO: Create a higher level API that contains all the repositories + const repoNode = graph.nodes({type: "REPOSITORY"})[0]; + const repo = new Repository(graph, repoNode.address); function issueOrPRByNumber(n: number): Issue | PullRequest { const result = repo.issueOrPRByNumber(n); if (result == null) { @@ -22,6 +24,11 @@ describe("GitHub porcelain API", () => { return result; } describe("has wrappers for", () => { + it("Repositories", () => { + expect(repo.url()).toBe("https://github.com/sourcecred/example-github"); + expect(repo.owner()).toBe("sourcecred"); + expect(repo.name()).toBe("example-github"); + }); it("Issues", () => { const issue = issueOrPRByNumber(1); expect(issue.title()).toBe("An example issue."); diff --git a/src/plugins/github/parser.js b/src/plugins/github/parser.js index 3005429..73234c0 100644 --- a/src/plugins/github/parser.js +++ b/src/plugins/github/parser.js @@ -9,6 +9,7 @@ import type { NodePayload, EdgePayload, PullRequestReviewNodePayload, + RepositoryNodePayload, AuthorNodePayload, AuthorsEdgePayload, PullRequestReviewCommentNodePayload, @@ -276,6 +277,8 @@ class GithubParser { const anyNode: Node = node; const type: NodeType = (node.address.type: any); switch (type) { + case "REPOSITORY": + break; case "ISSUE": case "PULL_REQUEST": const thisPayload: IssueNodePayload | PullRequestNodePayload = @@ -325,6 +328,16 @@ class GithubParser { } addRepository(repositoryJSON: RepositoryJSON) { + const repositoryPayload: RepositoryNodePayload = { + url: repositoryJSON.url, + name: repositoryJSON.name, + owner: repositoryJSON.owner.login, + }; + const repositoryNode: Node = { + address: this.makeNodeAddress("REPOSITORY", repositoryJSON.url), + payload: repositoryPayload, + }; + this.graph.addNode(repositoryNode); repositoryJSON.issues.nodes.forEach((i) => this.addIssue(i)); repositoryJSON.pullRequests.nodes.forEach((pr) => this.addPullRequest(pr)); } diff --git a/src/plugins/github/types.js b/src/plugins/github/types.js index 534fcd4..33f30d5 100644 --- a/src/plugins/github/types.js +++ b/src/plugins/github/types.js @@ -1,6 +1,13 @@ // @flow /** Node Types */ +export const REPOSITORY_NODE_TYPE: "REPOSITORY" = "REPOSITORY"; +export type RepositoryNodePayload = {| + +name: string, + +owner: string, + +url: string, +|}; + export const ISSUE_NODE_TYPE: "ISSUE" = "ISSUE"; export type IssueNodePayload = {| +url: string, @@ -60,6 +67,10 @@ export type AuthorNodePayload = {| // useful at the value layer as $ElementType, for // instance. export type NodeTypes = {| + REPOSITORY: { + payload: RepositoryNodePayload, + type: typeof REPOSITORY_NODE_TYPE, + }, ISSUE: {payload: IssueNodePayload, type: typeof ISSUE_NODE_TYPE}, PULL_REQUEST: { payload: PullRequestNodePayload, @@ -78,6 +89,7 @@ export type NodeTypes = {| |}; export type NodeType = + | typeof REPOSITORY_NODE_TYPE | typeof ISSUE_NODE_TYPE | typeof PULL_REQUEST_NODE_TYPE | typeof COMMENT_NODE_TYPE @@ -86,6 +98,7 @@ export type NodeType = | typeof AUTHOR_NODE_TYPE; export type NodePayload = + | RepositoryNodePayload | IssueNodePayload | PullRequestNodePayload | CommentNodePayload