* Define Reaction edges
This adds support to `github/edges` for creating edges representing
GitHub reactions. These edges are not actually added to the graph.
Test plan: Unit tests
* Add GitHub reactions to the graph
This commit adds functional support for reactions in SourceCred.
Only thumbs-up, heart, and hooray reactions are supported for now, as
they are all unambiguously positive; adding support for negative
reactions like thumbs-down will require some more thought.
The reactions are added to the graph, and new edge types have been added
to the UI.
Test plan:
The `graphView` class has been updated to do invariant checking for the
reaction edges, including that the unsupported reaction types like
"THUMBS_DOWN" aren't added to the graph.
I've tested this feature by downloading data for a large repository
(ipfs/go-ipfs). The reaction edges appear and transfer cred reasonably.
The edge types are displayed in the weight config appropriately.
Builds on #839, #840, and #845.
This adds support to `github/edges` for creating edges representing
GitHub reactions. These edges are not actually added to the graph.
Test plan: Unit tests
Summary:
In implementing #622, we’ll want to run lots of things inside of
transactions. This commit introduces a JavaScript API to do so more
easily, properly handling success and failure cases.
Test Plan:
Unit tests included, with full coverage; run `yarn unit`.
wchargin-branch: mirror-transaction-helper
Summary:
This affords more flexibility to clients, because an exact value can be
used in place of an inexact value, but not vice versa.
Test Plan:
Running `yarn flow` suffices.
wchargin-branch: schema-exact-type-fields
This commit updates the GitHub graphql query to also fetch reactions.
We update the JSON typedefs to include this new information, add
continuations from comments, and update existing continuation and query
code. Also, I added a safety check when updating comments for issues
that was previously unnecessary but is now needed.
Test plan:
- `yarn test --full` passes.
- Setting the page limits to 1 and running on the example-github does
not error with unexhausted pages, and loads all the expected reactions.
- Running on a larger repository (go-ipfs) works as expected.
- I have written dependent code that consumes these reactions in the
RelationalView, and works as intended, which suggests that the type
signatures are correct.
This commit updates the GitHub graphql query to also fetch reactions.
We update the JSON typedefs to include this new information, add
continuations from comments, and update existing continuation and query
code. Also, I added a safety check when updating comments for issues
that was previously unnecessary but is now needed.
Test plan:
- `yarn test --full` passes.
- Setting the page limits to 1 and running on the example-github does
not error with unexhausted pages, and loads all the expected reactions.
- Running on a larger repository (go-ipfs) works as expected.
- I have written dependent code that consumes these reactions in the
RelationalView, and works as intended, which suggests that the type
signatures are correct.
Now that #832 gave us logic to parse references to commits, we have the
RelationalView find and add these references. The actual change is
a simple extension of existing reference detection logic.
Test plan: Observe that the snapshots are updated with references to
commits from the example-github repository.
Progress on #815.
We add a new function, `findCommitReferences`, which can find both
explicit url references to commits, and commit hashes.
Since the commit url includes the commit hash, some extra logic is added
to deduplicate them in this instance. Tests verify that this is done
properly.
Test plan: Unit tests cover the cases of having commit hashes, having
commit urls, and having both at once.
Summary:
GraphQL unions are required to be unions specifically of object types.
They cannot contain primitives or other union types as clauses. This is
good: it means that we don’t have to worry about unions that recursively
reference each other or themselves.
Unions are also required to have at least one clause, but we don’t
validate this because it’s not helpful for us. An empty union is
perfectly well-defined, if useless, and shouldn’t cause any problems.
Relevant portion of the spec:
<https://facebook.github.io/graphql/October2016/#sec-Union-type-validation>
Test Plan:
Unit tests added, retaining full coverage; `yarn unit` suffices.
wchargin-branch: graphql-schema-union-validation
Summary:
This commit introduces a module for declaratively specifying the schema
of a GraphQL database. See `buildGithubSchema` in `schema.test.js` for
an example of the API.
This makes progress toward #622, though the design has evolved some
since its original specification there.
Test Plan:
Unit tests added, with full coverage; `yarn unit` suffices.
wchargin-branch: graphql-schema
There was a bad interaction between #830 and #829, wherein they both
independently changed the snapshot. So they passed individually, and
failed once both merged together. This fixes it.
Test plan: `yarn test --full` passes.
Now that the GitHub plugin knows about commit messages (#828), we can
parse those commit messages to find references to other GitHub entities.
Fixed a minor typing mistake along the way.
Test plan:
Observe that a number of references have been detected among the commits
in the example GitHub repository. We mistakenly find references to
wchargin because we don't have a proper tokenizer. (#481)
Progress on #815.
We could get this information from the Git plugin, but since we want to
use this for reference detection, it's much easier to have this follow
the same pipeline as all the other GitHub reference detection code.
I've updated the relational view to also remove the commit messages when
compressing by removing bodies. A unit test was added to check this
works as intended.
See #815 for tracking.
Test plan:
`yarn test --full` passes.
Snapshot changes are appropriate.
In #824, we loaded every commit in the default branch's history into the
GitHub relational view, along with authorship info. This commit actually
uses that authorship info to create AUTHORS edges from the commit to the
user that authored it (whenever possible).
The implementation is quite simple: we just need to yield the commits
when we yield all the authored entities, so that we will process their
authors and add them to the graph. Also, I updated the invariant
declarations in `graphView.js`, and corrected a type signature so that the
new invariants would typecheck.
Test plan: The snapshot update shows that commits are being added to the
graph appropriately. Observe that commits which do not have a valid
GitHub user as their author do not correspond to edges in the graph.
See [example].
This is basically a solution to #815, but I'll defer closing that issue
until I've added a few more features, like reference detection.
[example]: 6bd1b4c0b7
This builds on #821 so that every commit in the default ref's history is
added as a Commit entity to the GitHub relational view. This means that
these commits are also added to the graph by the GitHub plugin. In
general, this will have no effect on real graphs, because these commits
were already available via the Git plugin.
Test plan:
Observe that the snapshot changes just correspond to new commits being
available to the RelationalView, and correspondingly added to the GitHub
graph. `yarn test --full` passes.
GitHub has a procedure for encoding node addresses into sequences of
string "parts", so that we can generate unique edge addresses. Right
now, the encoding strategy assumes that when we encode a node address
into parts, that node address always starts with the prefix
`["sourcecred", "github"]`. However, #816 makes the Git commit address a
valid GitHub address, which means that this assumption no longer holds.
We could start adding special-cased logic to ensure that we de-serialize
Commit addresses properly, but what if we create edges between GitHub
entities and other plugins' nodes in the future? It is much cleaner to
remove the assumption, and serialize the full node address as parts in
the edge address. This makes the GitHub edge addresses somewhat longer,
but this is OK for now as we don't ever store those on disk. If, in the
future, node/edge address length is a problem, we can investigate more
principled and maintainable compression strategies at that time.
Test plan: `yarn test --full` passes.
This adds logic for retrieving every commit in the default branch's
history, along with authorship information connecting that commit to a
GitHub user (when available).
This will allows us to do better cred tracking, especially for projects
that don't always use pull requests for merging code.
This results in a moderate increase in load time for the GitHub plugin.
On my machine, loading SourceCred before this change takes 30s, and
after this change it takes 34s.
Test plan:
Observe that the example-github has been updated with commits and
authorship. Also, I ran the query for a larger repository
(`sourcecred/sourcecred`) to verify that the continuation logic works.
This adds a `Commit` entity to the GitHub relational view. It has all
the standard methods: commits can be retrieved en masse or by particular
address, they have a URL and authors, and (de)serialize appropriately.
The code for adding pull requests has been modified so that the merge
commits are added as commit entities. This does not have any effect on
the ultimate graph being created; the same edge is added either way.
Test plan: I've extended the standard RelationalView tests to cover the
`Commit` entity. The case where the commit has 0 authors is not yet
tested, but will be once I add support for getting all of the commits
from the example-github (we have one example of a commit that doesn't
map to a user).
Progress on #815.
The Git plugin owns Commits, but the GitHub plugin also creates commits.
This commit reifies that relationship by making a Git commit address a
valid GitHub structured address. This is precursor work for #815, which
will require adding a commit entity to the GitHub relational view.
Also, this commit surfaces and fixes a minor type bug, wherein a map
from strings to referent addresses was typed to hold any structured
address, rather than just referent addresses.
Test plan: The unit tests confirm that serializing/deserializing a Git
commit address using the GitHub plugin's methods works as intended.
Also, unit tests were added that verify that (de)serializing Git
addresses for non-commit objects is still an error.
This commit pulls the graphql fields to request commit information into
a fragment, and requests GitHub authorship information (when
available) for that fragment. We don't use that information yet, but we
will soon. Progress on #815.
Test plan: Observe that the example-github data is updated, so that we
now have urls and authorship for commits. Observe that the query has
updated, but no downstream code was affected. `yarn test --full` passes.
Both the GitHub and Git plugins create a `_Prefix` object for nodes and
edges, which gives the respective prefixes for different node/edge
types. We named it `_Prefix` because we weren't sure if these should be
exported. In practice, these have proven quite useful to make generally
available, and despite the `_`-naming we expose the objects outside
their modules. This change renames `_Prefix` to `Prefix` to reflect the
reality that these are used as public consts.
Exporting them is safe as both objects are frozen.
Test plan: Simple rename, `yarn test` suffices.
This commit builds on the work in #806, adding the
`MentionsAuthorReference`s to the graph. It thus resolves#804.
Empirically, the addition of these edges does not change the users' cred
distribution much. Consider the results with the following 3 forward
weights for the edge (results for ipfs/go-ipfs):
| User | w=1/32 | w=1/2 | w=2 |
|---------------|-------:|-------:|-------:|
| whyrusleeping | 228.04 | 225.69 | 223.86 |
| jbenet | 102.04 | 100.26 | 99.53 |
| kubuxu | 66.60 | 67.80 | 69.36 |
| ... | — | — | — |
| btc | 22.69 | 22.29 | 21.38 |
The small effect on users' cred is not that surprising: the
MentionsAuthor references always "shadow" a direct comment->user
reference. In principle, the overall cred going to the user should be
similar; the difference is that now some more cred flows in between the
various comments authored by that user, on the way to the user. (And if
those other comments had references, then it flows out from them, etc.)
Empirically, the variance on comments' scores seems to increase as a
result of having this heuristic, which is great—the fact that all
comments had about the same score was a bug, not a feature.
Sadly, we don't have good tooling for proper statistical analysis of the
effect this is having. We'll want to study the effect of this heuristic
more later, as we build tooling and canonical datasets that makes that
analysis feasible.
We choose to add this heuristic, despite the ambiguous effect on users'
cred, because we think it is principled, and adds meaningful structure
to the graph.
Test plan:
The commit is a pretty straightforward generalization of our existing
GitHub edge logic. All of the interesting logic was thoroughly tested in
the preceding pull, so this commit just tests the integration. Observe
that standard (de)serialization of the edge works, that the snapshot is
updated with a MentionsAuthor reference edge, and that the graph
invariant checker, after update, does not throw errors. Also, I manually
tested this change on the ipfs/go-ipfs repo. (It does not require
regenerating data.)
A `MentionsAuthorReference` is created when a post mentions a user, and
that user has authored at least one post in the same thread. Then there
is a `MentionsAuthorReference` from the post to the other posts by that
author.
For context, see the docstrings in `mentionsAuthorReference.js`, and
see #804.
Test plan:
Thorough unit tests have been added, which test the entire pipeline,
from ingesting the data via GitHub's graphql responses, through to
detecting the references. Edge cases such as self-reference and
multi-reference are tested.
Thanks to @wchargin for help writing this commit.
With some frequency we find ourselves needing to maintain maps whose
values are arrays that we append to. `MapUtil.pushValue` is a utility
method for these cases.
Existing usage in `aggregate.js` has been modified to use the new
function.
Test plan: Unit tests included.
Summary:
Per #800, each test file should start with a `describe` block listing
its file path under `src`. Currently, nine of our tests do not do so.
Of these, eight had a top-level describe block with the wrong name
(either not a filepath or an outdated filepath), while only one short
test was missing a top-level describe block altogether. This patch fixes
each file to use the correct format.
Test Plan:
Apply the Sharness test in #802, and note that it fails before this
patch but passes after it.
wchargin-branch: describe-fix
Previously, the WeightConfig (and the button that expanded it) were in
the credExplorer App. This was a little weird, as there's no reason to
play with the weights before you have some Pagerank results to
investigate; additionally, it risked confusing new users with a concept
that was not yet applicable.
Also, the implementation was wonky: the WeightConfig had responsibility
for expanding/hiding itself, which gave poor ability to position the
button and the WeightConfig separately.
Finally, the codepath was untested (vestiges of #604).
This commit fixes all three issues:
- The WeightConfig and button have moved into PagerankTable
- The WeightConfig is now a stateless component, and the parent takes
responsibility for deciding when to mount it
- Logic for showing/hiding the WeightConfig is now tested.
This commit implements a [suggestion] to make `credExplorer/App` a
single source of truth on the `WeightedTypes`. As such, both
`WeightConfig` and `PluginWeightConfig` have been refactored to be
(essentially) stateless components that are controlled from above. I say
essentially because `WeightConfig` still has its expanded state, but
that will go away soon.
Along the way, I've improved testing and added some new invariant
checking (e.g. that `PluginWeightConfig` was passed the correct set of
weights for its adapter). For the first time, there are now tests for
the `WeightConfig` itself! I'm not totally done with the weight
re-write, but this seems like a good time to close#604, as the whole
logical sequence for setting weights is now tested.
Test plan: There are new unit tests. Also, to be sure, I manually tested
the UI as well.
[suggestion]: https://github.com/sourcecred/sourcecred/pull/792#issuecomment-419234721
This commit refactors `credExplorer/App` so that instead of storing an
`EdgeEvaulator` in its state, it stores `WeightedTypes` instead. This
has a few benefits:
- It's trivial to generate the right default value for `WeightedTypes`,
so we no longer allow the variable to be nullable in the state. This
simplifies logic, removes an error case, and means that we don't require
the `WeightConfig` to mount before the app is usable.
- `WeightedTypes` are serializable and can be tested for equality, so
they are a better-behaved piece of state
- We put off the information-destroying transformation as long as
possible
- In the future, I think we may want to move the weights/types concept
into core, at which point the `WeightedTypes` will directly be consumed
by the `core/attribution` module.
Test plan: Unit tests are pretty thorough; to be safe, I tested the UI
myself.
This refactors PluginWeightConfig so that it uses the
`defaultWeightsForAdapter` method introduced in #787.
The refactor is mildly invasive, as we switch the state from being a
(mutable) `WeightedTypes` to having a (regular, read-only)
`WeightedTypes`. I think this is an improvement in consistency.
Test plan: Trivial refactor; unit tests+flow pass.
This commit creates a central `weights` module that defines all of the
weight-related types, and provides some utilities for dealing with them.
This way users of weight-concepts do not need to depend on a lot of
random modules just to get the relevant types. The utility methods are
implicitly defined a few places in the codebase: now we can avoid
re-writing them, and test them more thoroughly.
Test plan: Unit tests pass.
Currently, the `credExplorer` uses the `defaultStaticAdapters`, but it
imports these adapters in multiple places. If we decide to make the
adapters configurable (e.g. when we start supporting more plugins) this
will be a problem.
This change modifies the cred explorer so that the adapters always come
from a prop declaration on the app. Then the adapters are passed into
the `state` module's functional entry points, rather than letting
`state` depend on the default adapters directly.
This change is motivated by the fact that my WeightConfig cleanup can be
done more cleanly if the adapters are present as a prop on the App.
Test plan: Unit tests are updated. Also, `git grep
defaultStaticAdapters` reaveals that the adapters are only consumed
once.
This commit adds `weightsToEdgeEvaluator`, a function for converting
weighted node types into an `EdgeEvaluator`. This replaces the
`edgeWeights` module (which was untested, and an outmoded API).
Test plan: The new `weightsToEdgeEvaluator` method is well-tested.
Since `WeightConfig` is still not tested, I manually verified that it
still works as anticipated.
Summary:
Lots of tests need the output of `yarn backend`. Before this commit,
they tended to create it themselves. This was slow and wasteful, and
also could in principle have race conditions (though in practice usually
tended not to).
This commit updates tests to respect a `SOURCECRED_BIN` environment
variable indicating the path to an existing directory of backend
applications.
Closes#765.
Test Plan:
Running `yarn test --full` passes.
Prepending `echo run >>/tmp/log &&` to the `backend` script in
`package.json` and running `yarn test --full` results in a log file
containing only one line, indicating that the script really is run only
once.
wchargin-branch: deduplicate-backend
This is convenient for testing other code, where we may want to directly
use the fallback types. One test has been updated in this way.
I also changed the names for the fallback adapter's edges to be somewhat
more readable.
Test plan: Tests improved.
This commit adds PluginWeightConfig, which is responsible for
adding all the weights for an individual plugin. The top-level
WeightConfig now creates multiple PluginWeightConfigs. It also takes
responsibility for hiding the FallbackPlugin.
Test plan: The PluginWeightConfig is tested (and fairly simple). The
top-level WeightConfig is not yet tested (#604), so I manually tested
that the weights in the app still function.
`testUtil.configureEnzyme` now additionally asserts, after every test,
that `console.error` and `console.warn` were not called. Tests that
explicitly expect such calls can still be written by manually re-mocking
the relevant console method (and several examples already exist).
The code that explicitly specifies this for various enzyme test files
has been removed.
Test plan: `git grep "not.toHaveBeenCalled"` shows only unrelated usage.
`yarn test` passes. Adding a spurious console.warn to a passing test
causes it to fail.
Fixes#668
Summary:
This simplifies interfaces everywhere.
See also #216, which did the opposite of this as a temporary fix due to
a Babel/Webpack interaction that no longer exists as of #766.
Test Plan:
Note that `node bin/sourcecred.js load sourcecred/example-git` still
works (after `yarn backend`). Note that `yarn test` still works. These
demonstrate that the module works from both a Webpack context and a Node
context. Note that `git grep --name-only execDependencyGraph` yields
exactly those files touched in this commit. Note that `yarn test --full`
passes.
wchargin-branch: commonjs-execDependencyGraph
Currently, it's possible to lock the cred explorer UI via the following
sequence of actions:
1. Press the analyze cred button
2. While it is running, modify the weights
After following this repro, the UI is stuck: it will say "loading"
forever, and the analyze cred button is disabled.
The issue is that loadGraph and runPagerank do not apply their success
(or failure) state transitions if they are pre-empted by another state
change. If a repo change occurs, that's the right behavior: the repo
change puts the state back to `"READY_TO_LOAD_GRAPH"`, which means the
UI is ready to re-load, and showing the PageRank results for the wrong
repo would be very confusing.
However, if an edge evaluator change occurs while loadGraph or
runPagerank is happening, the state is left in the "LOADING" status
(which means the analyze cred button is disabled).
This commit fixes the issue via a refactor: per @wchargin's suggestion,
responsibility for the edge evaluator moves from the state module out to
`credExplorer/App.js`. This dramatically simplifies the state module, as
we no longer need a `Substate` concept: we can simplify the state into a
single sequence of states.
As of the refactor, the bug is impossible.
Test plan: Unit tests have been updated to maintain coverage on the
refactored code. I manually tested that the bug no longer repro's, and
that the cred explorer UI continues to function. I did not add a new
test to protect against regression, because in the new codepath, the bug
is basically impossible.
This commit modifies the edge type so that it has a
`defaultForwardWeight` and `defaultBackwardWeight`, and these defaults
are respected by the `WeightConfig`.
I came up with reasonable-seeming defaults for the Git and GitHub
plugins; these will undoubtably be more methodically tuned in the
future.
Test plan: `yarn test` passes, also opening the cred explorer now has
the specified default weights in the WeightConfig. (Note that the
forward/backward directions are reversed as described in #749.)
This commit introduces a new component, `EdgeTypeConfig`, which is
responsible for configuring the weights for a given edge type. The
config creates two `WeightSlider`s: one for the forward direction, and
one for the backward direction. The `DirectionalitySlider` is no longer
used, and is removed. This fixes#596.
So as to avoid confusion, we now describe every edge with variables, as
in 'α REFERENCES β', and clarify that the weight modifies how cred flows
from β to α. This necessitated the creation of an `EdgeWeightSlider`,
local to the `EdgeTypeConfig`, which sets up a `WeightSlider` with the
necessary greek characters.
The EdgeTypeConfig is tested, so this is continuing progress towards
solving #604.
Test plan: I manually verified that modifying edge weights has the
expected effect on cred scores. Also, some new unit tests are included.
This factors `NodeTypeConfig` out of the `WeightConfig` component. The
scope for a `NodeTypeConfig` is that it configures a single node type.
Right now it just renders a single `WeightSlider`, but I like factoring
out both for consistency with the `EdgeTypeConfig` (see #749) and
because I expect we may want to add more complexity later.
Test plan: The new component has some tests, also I manually tested the
frontend.
