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

add discourse references to the graph #1410

Merged
merged 1 commit into from Oct 18, 2019
Merged

add discourse references to the graph #1410

merged 1 commit into from Oct 18, 2019

Conversation

@decentralion
Copy link
Member

decentralion commented Oct 11, 2019

This commit modifies discourse/createGraph so that it finds all of the
same-server Discourse references in Discourse posts, and creates
appropriately typed references edges in response.

The unit tests have been updated with cases for both references that
should exist, and references that shouldn't (e.g. post index out of
bounds, or a reference to the wrong server).

Test plan: yarn test --full along with snapshot update.

@decentralion decentralion force-pushed the dev-ref-6 branch from 5f4261e to d40d82f Oct 11, 2019
@decentralion decentralion changed the base branch from dev-ref-6 to master Oct 17, 2019
@decentralion decentralion force-pushed the dev-ref-7 branch from f0468b6 to 80fa811 Oct 17, 2019
@decentralion decentralion requested review from Beanow and wchargin Oct 17, 2019
@@ -199,4 +223,67 @@ class _GraphCreator {
}
}
}

referenceEdge(post: Post, reference: DiscourseReference): Edge | null {
if (reference.serverUrl !== this.serverUrl) {

This comment has been minimized.

Copy link
@Beanow

Beanow Oct 17, 2019

Member

I think protocol and domain name are case-insensitive. Probably a good idea to lowercase both sides of this comparison. Maybe include mixed case in a smoke test.

This comment has been minimized.

Copy link
@decentralion

decentralion Oct 18, 2019

Author Member

Good point. I fixed this and added a test case.

const discourseReferences = linksToReferences(parseLinks(post.cooked));
for (const reference of discourseReferences) {
const edge = this.referenceEdge(post, reference);
if (edge != null) {
this.graph.addEdge(edge);
}
}
Comment on lines +193 to +199

This comment has been minimized.

Copy link
@Beanow

Beanow Oct 17, 2019

Member

In theory, we should only add unique edges, right? Do we need to remove duplicates here?

This comment has been minimized.

Copy link
@decentralion

decentralion Oct 17, 2019

Author Member

From per the docs for Graph.addEdge: addEdge is idempotent if the same edge is added multiple times. So we don't need to do anything special here. :)

/**
* Add an edge to the graph.
*
* It is permitted to add an edge if its src or dst are not in the graph. See
* the discussion of 'Dangling Edges' in the module docstring for semantics.
*
* It is an error to add an edge if a distinct edge with the same address
* already exists in the graph (i.e., if the source or destination are
* different).
*
* Adding an edge that already exists to the graph is a no-op. (This
* operation is idempotent.)
*
* Returns `this` for chaining.
*/
addEdge(edge: Edge): this {

This commit modifies `discourse/createGraph` so that it finds all of the
same-server Discourse references in Discourse posts, and creates
appropriately typed references edges in response.

The unit tests have been updated with cases for both references that
should exist, and references that shouldn't (e.g. post index out of
bounds, or a reference to the wrong server).

Test plan: `yarn test --full` along with snapshot update.

This is progress towards [Discourse reference and mention detection][1].

[1]: https://discourse.sourcecred.io/t/discourse-reference-mention-detection/270
@decentralion decentralion force-pushed the dev-ref-7 branch from 80fa811 to 7d53a4f Oct 18, 2019
@decentralion decentralion merged commit b294339 into master Oct 18, 2019
2 checks passed
2 checks passed
ci/circleci: publish-1 Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@decentralion decentralion deleted the dev-ref-7 branch Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.