This commit removes the node and edge counts that are displayed on the
cred explorer. While this is nice information to surface, I think it's
currently surfaced in the wrong place: it should be displayed as part of
the PagerankTable.
My proximate motivation for removing it is that it cleans up the data
structure slightly and I'm about to intensively refactor the whole file.
Test plan: `yarn test`; also, I manually engaged the cred explorer and
it still works properly.
Our data model orients on getting repos from GitHub, which are
alternatively represented as strings like "sourcecred/sourcecred", or
pairs of variables representing the owner and name, or objects with
owner and name properties. We also have a few different implementations
of repo validation, which are not applied consistently.
This commit changes all that. We now have a consistent Repo type which
is an object containing a string owner and string name. Thanks to a
clever suggestion by @wchargin, it is implemented as an opaque subtype
of an object containing those properties, so that the only valid way to
construct a Repo typed object is to use one of the functions that
consistently validates the repo.
As a fly-by fix, I noticed that there were some functions in the GitHub
query generation that didn't properly mark arguments as readOnly. I've
fixed these.
Test plan: No externally-observable behavior changes (except insofar as
there is a slight change in variable names in the GitHub graphql query,
which has also resulted in a snapshot diff). `yarn travis --full`
passes. `git grep repoOwner` presents no hits.
Test Plan:
None really needed—the infrastructure has already been tested—but you
can verify that this works both under `yarn start` and `yarn build` by
navigating to the evident URL.
wchargin-branch: discord-invite
Summary:
This patch extends our routing infrastructure to add support for
_external_ redirects. It does not include dedicated support for
site-internal redirects.
Test Plan:
Add an external redirect to `routeData`, like the following:
```diff
diff --git a/src/app/routeData.js b/src/app/routeData.js
index 83dff72..eaba130 100644
--- a/src/app/routeData.js
+++ b/src/app/routeData.js
@@ -36,6 +36,15 @@ const routeData /*: $ReadOnlyArray<RouteDatum> */ = [
title: "SourceCred explorer",
navTitle: "Explorer",
},
+ {
+ path: "/discord-invite",
+ contents: {
+ type: "EXTERNAL_REDIRECT",
+ redirectTo: "https://discord.gg/tsBTgc9",
+ },
+ title: "SourceCred Discord invite",
+ navTitle: null,
+ },
];
exports.routeData = routeData;
```
Then:
- run `yarn build`, and:
- verify that the appropriate `index.html` file is correctly
generated;
- verify that opening the `index.html` file in a browser redirects
you to the appropriate destination, even with JavaScript
disabled;
- verify that the link in the body of the HTML page is correct
(easier to do if you remove the `<meta>` tag)
- run `yarn start`, and:
1. use the React DevTools to change the “Explorer” link’s `to` prop
from `/explorer` to `/discord-invite`;
2. click the link; and
3. verify that you are properly redirected.
wchargin-branch: add-external-redirect
@wchargin suggested displaying scores this way. This way, lowest scores
are best, and higher scores are worse. This is a little
counterintuitive, but maybe less counterintuitive than the current
approach, which arbitrarily adds 10 to scores to keep them non-negative,
and results in an arbitrary crossing point where scores become negative
without any particular significance.
Test plan: Travis, and manual inspection of the frontend.
Summary:
In addition to a routine libdef update, we also need to work around a
particularly nasty new bug in Flow, which requires `any`-casts that are
even more unsafe than usual. That said, I think that it’s worth that
cost to remain up to date with Flow, so that we can amortize future such
issues.
Test Plan:
Running `yarn travis --full` passes.
wchargin-branch: upgrade-flow-v0.76.0
Some slight changes were needed to the other test code to avoid spurious
errors. Specifically, we now always set up a mocked fetch response in
non-failure cases, even if we don't wait for it to resolve.
Test plan: I manually tested it, also see the new unit tests.
Modifies the PluginAdapter interface so that NodeTypes come with
default weights, and modify the WeightConfig so that it loads those
NodeTypes as the default weights.
The new weight choices are not super principled, but are clearly better
than the uniform default. Now, projects find that most pull requests are
more valuable than most git blobs. :)
Sadly, the WeightConfig does not yet have any tests, so there are no
test changes.
Test plan: I manually verified that it works as expected, by clearing
application data and reloading the cred explorer.
Currently, the GitHub graph fetcher will characteristically fail if:
1. it times out GitHub's server
2. it triggers the semidocumented abuse detection mechanism
In case 1, an intelligible error is posted to the console. In case 2, it
produces an unintelligible TypeError, because the response is not a
valid GraphQL response (the error field is not populated; it has a
custom message instead).
As of this commit, we gracefully catch both cases, and print a message
to console directing the user to #350, which has context on GitHub query
failures. This new catch works because in case 2, the data field is
empty, so we now properly recognize `x.data === undefined` as an error
case.
Thanks to @wchargin for the investigatory work behind this commit.
Fixes#223.
Test plan:
We don't have unit tests that cover this case, but I did manually test
it by asking GitHub to fetch `ipfs/go-ipfs`, which consistently fails.
I also tested it by using an invalid length-40 GitHub API token.
Summary:
There have been a couple of occasions on which we’ve considered using
it, but didn’t want to require from `app/`.
Test Plan:
Unit tests added, with full coverage.
wchargin-branch: extract-dedent
In #529, I made the cred explorer populate a dropdown with the list of
repositories that are available to explore. That dropdown defaults to
selecting the alphabetically first repository.
This has an unfortunate consequence in that it makes it impossible for
us to explicitly set a default - for example, we would like
sourcecred.github.io/explorer to show sourcecred/sourcecred by default,
but instead it shows example-git.
So that we can choose the default, I've changed the logic so that it
instead shows the most-recently-loaded data first. This required
a breaking change to the repoRegistry serialized format, so I've also
refactored the module to use compat, which I should have done from the
beginning.
Test plan:
Unit tests for the repo selector are updated. The CLI load command
unfortunately has no tests, so I manually tested that it always provides
the lastest repository last, and appropriately handles the case where
the same repository is loaded multiple times.
Context: The Cred Explorer loads data (currently on a per-repository
basis) that has previously been prepared by running the `sourcecred
load` cli command.
Currently, to select a repository to load, the user must manually type
the repository owner and name. This is a confusing UI, because it
suggests that any repository may be chosen, when in fact only repos
already loaded into the data store are available. The user is given no
feedback as to which repositories are valid options.
As of #516, the backend stores a registry listing available
repositories. This commit adds a `RepositorySelect` component which
loads the available from that registry, and makes them available in a
dropdown, in sorted order.
When the user manually selects one of the repositories, that selection
is persisted into localStorage and respected on future loads. If the
user hasn't made such a choice, then the first repository is selected by
default.
The implementation is highly influenced by testability considerations.
The default export, `<RepositorySelect onChange={} localStore={} />`, is
pretty straightforward. The `RepositorySelect` is somewhat cumbersome to
test because it asynchronously fetches data and then updates its state,
which affects the render output. So as to avoid testing inside async
react components wherever possible, I've factored out:
* `loadStatus`, which uses fetch and localStore to get the status of the
selector.
* `PureRepositorySelect`, which just renders a `Status`, such as
loading, failure, or valid
* `LocalStoreRepositorySelect`, which wraps the `PureRepositorySelect`
with logic to bind the repository select to localStore on change.
Test plan: Extensive unit tests were added. Also, to ensure that the
tests were testing the right thing, I manually tested:
- attempting to load invalid registry
- attempting to load with no registry
- attempting to load with empty registry
- loading without valid localStore
- changing the setting via dropdown
- loading from localStore after changing the dropdown
And all behavior was as expected.
Thanks to @wchargin for considerable help testing this PR.
Summary:
Test code should probably always use a checked, memory-backed local
storage implementation. This endpoint will help users not forget to
include the checks.
wchargin-branch: test-local-store
Summary:
Might as well have runtime type safety, in case we accidentally try to
store any more `Map`s or `undefined`s.
Test Plan:
Tests pass, but are likely not sufficient. Manual testing indicates that
the local storage still works, for both reads and writes, on a fresh
profile or with existing data, for both the repository owner/name and
the weight configuration.
wchargin-branch: use-checked-local-store
Summary:
We can use this in tests. If need be, we can enhance this class to allow
simulating failures, low storage limits, etc., but just having a pure
implementation at all is all we need right now.
Test Plan:
Unit tests added.
wchargin-branch: memory-local-store
Summary:
This provides some extra checking around `LocalStore` calls. In
particular, it fails fast on the nasty bug where storing a `Map`
actually stores the empty object (`JSON.stringify(new Map()) === "{}"`).
Similarly, retrieving a value that was stored as `undefined` will raise
an error, because `JSON.parse(JSON.stringify(undefined))` raises an
error.
This should have negligible performance impact—local storage access
should never be on a critical path. We can choose to elide this in
production if we want.
Test Plan:
Unit tests added. Manual testing of the cred explorer yields no errors.
wchargin-branch: checked-local-store
Summary:
This commit modifies components that directly depend on the
browser-specific local store implementation to instead have their
dependencies injected.
Test Plan:
Tests pass, but are likely not sufficient. Manual testing indicates that
the local storage still works, for both reads and writes, on a fresh
profile or with existing data, for both the repository owner/name and
the weight configuration.
wchargin-branch: di-localstore
Summary:
We’d really like to be able to test components that use `LocalStore`. We
can do this by dependency-injecting the storage backend. This commit
begins that process by extracting `LocalStore` to its interface,
preserving the unique existing implementation.
wchargin-branch: extract-localstore
Summary:
This commit switches to a double-buffered PageRank implementation. When
benchmarked on `ipfs/js-ipfs`, the critical section improves from
3059 ms to 2433 ms (79.5% of original), and peak heap usage drops from
342 MB to 207 MB. (Tested non-rigorously in Chrome 67.)
Test Plan:
Existing unit tests for `sparseMarkovChainAction`,
`findStationaryDistribution`, and `pagerank` are sufficient.
wchargin-branch: pagerank-dbuf
Summary:
The PageRank functions can take a long time to compute. We’d like them
to not lock the browser, and we’d also like them to communicate with
their clients (e.g., to update a progress bar). This code updates
`findStationaryDistribution` and downstream `pagerank` to return
promises.
Test Plan:
Unit tests updated. The cred explorer (`yarn start`) still works.
Applying
```diff
diff --git a/src/core/attribution/markovChain.js b/src/core/attribution/markovChain.js
index 2acce9c..c7a7159 100644
--- a/src/core/attribution/markovChain.js
+++ b/src/core/attribution/markovChain.js
@@ -166,6 +166,7 @@ export function findStationaryDistribution(
return;
}
} while (Date.now() - start < yieldAfterMs);
+ console.log("Yielding.");
setTimeout(tick, 0);
};
tick();
```
causes the appropriate log messages to be printed in the browser—about
once every ten iterations for `sourcecred/sourcecred`.
wchargin-branch: asynchronous-pagerank
We want the UI to offer a list of available repositories, rather than
using a text input box. To do this, we first need the backend to include
a registry of all available repositories.
Test plan:
Sadly we don't have CLI testing, so I manually verified this by doing
the following:
```
$ yarn backend
$ rm -r $SOURCECRED_DIRECTORY
$ node bin/sourcecred.js load sourcecred example-github
$ cat $SOURCECRED_DIRECTORY/repositoryRegistry.json
{"sourcecred/example-github":true}
$ node bin/sourcecred.js load sourcecred example-github
$ cat $SOURCECRED_DIRECTORY/repositoryRegistry.json
{"sourcecred/example-github":true}
$ node bin/sourcecred.js load sourcecred example-git
$ cat $SOURCECRED_DIRECTORY/repositoryRegistry.json
{"sourcecred/example-git":true,"sourcecred/example-github":true}
```
Previously, WeightConfig hackily contained its own enumeration of all
node and edge types. Now, it loads them from the StaticPluginAdapter.
Test plan:
Unit tests pass, as does manual inspection of the frontend.
In some cases (e.g. WeightConfig) we want to have information from the
PluginAdapater before loading any data from the server. In other cases,
we need to combine the PluginAdapater with actual data, e.g. so we can
get the description of a GitHub node.
To support this, we split the PluginAdapter into a Static and Dynamic
component. The Dynamic component has data needed to give node
descriptions, etc. Given a static adapter, you can get a promise to load
the dynamic adapter. Given the dynamic adapter, you can immediately get
the static adapter. (There's a parallel to NodeReference (static) and
NodePorcelain (dynamic)).
Test plan:
Travis passes, as does manual testing of the frontend.
- PluginAdapters no longer expose a Renderer; instead, the render
methods are inlined on the PluginAdapter. The extra abstraction didn't
provide any lift in the current architecture.
- The edgeVerb function has been removed.
- PluginAdapters now enumerate EdgeTypes. Each has a prefix, and a
forward and a backward name.
Test plan: `yarn travis`, plus manual testing of the frontend and the
weight config.
Summary:
We don’t need this to be a “progressive web app”—certainly not now. The
n+1 caching problem is not a good tradeoff for us, and furthermore
service workers are causing flashes of content on server-side rendered
pages.
This commit is a quick fix to remove them. We can remove the code
entirely if we want, or just keep it as is.
Test Plan:
On a machine has the service worker registered, run `yarn build`, then
`node bin/sourcecred.js start`. Note in the network panel that the
service worker is loaded on the first page load, but then deregistered.
On subsequent refreshes, it should not activate. In the “Application”
panel of the Chrome dev tools, it should appear as “deleted”.
wchargin-branch: disable-sw
The WeightConfig is a power user feature. Now that we're building a
public-facing demo out of the Cred Explorer, it will be better to hide
the weight configuration by default.
This commit adds a button for showing/hiding the weight configuration.
The weights are still propagated correctly regardless of whether the
weight config is shown.
Test plan:
- Ensure that the site loads with weights hidden by default.
- Ensure that clicking the button causes the weight config to display.
- Ensure that PageRank loads and displays correctly with the weights
hidden.
- Ensure that changes to the weight config still propagate to PageRank
(with weights hidden or not hidden).
Test Plan:
Ensure that `require("./src/app/routeData")` works in `node` without any
preprocessing. Ensure that `yarn start` works, and that `yarn build`
then `node ./bin/sourcecred.js start` also works.
wchargin-branch: vanilla-route-data
Summary:
Some of the code here is adapted from my site (source available on
GitHub at wchargin/wchargin.github.io). It has been improved when
possible and made worse when necessary to fit into our existing build
system with minimal churn.
As of this commit, there remain the following outstanding tasks:
- Use a non-hardcoded list of paths in static site generation router.
This is not trivial. We have the paths nicely available in
`routes.js`, but this module is written in ES6, and transitively
depends on many files written in ES6 (i.e., the whole app). Yet
naïvely it would be required from a Webpack config file, which is
interpreted as vanilla JavaScript.
- Add `csso-loader` to minify our CSS. This is easy.
- Add unit tests for `dedent`. (As is, it comes from my site
verbatim. I wrote it. dmnd’s `dedent` package on npm is insufficient
because it dedents arguments as well as the format string, which is
incorrect at least for our purposes.)
- Link in canonical static data for the site.
- Rip out the whole build system and replace it with my build config,
which is orders of magnitude saner and less bad. (By “the whole
build system” I mostly mean `webpack.config.{dev,prod}.js`.)
Test Plan:
```shell
$ yarn backend
$ yarn build
$ node ./bin/sourcecred.js start
```
wchargin-branch: static-v0
Summary:
This adds a dummy landing page. We’ll want to actually put nice content
on it. For development convenience, I’m totally fine with having the
`yarn start` launch `/explorer` instead of just `/`.
Test Plan:
Run `yarn start` and note that the navigation works.
wchargin-branch: landing-page
Summary:
This commit hooks up the PageRank table to the PageRank node
decomposition developed previously. The new cred explorer displays one
entry per contribution to a node’s cred (i.e., one entry per in-edge,
per out-edge, and per synthetic loop), listing the proportion of the
node’s cred that is provided by this contribution. This makes it easy to
observe facts like, “90% of this issue’s cred is due to being written by
a particular author”.
Paired with @decentralion.
Test Plan:
Unit tests added; run `yarn travis`.
wchargin-branch: pagerank-table-node-decomposition
Summary:
The aesthetically nicest win is in `WeightConfig`. Other changes are
nice to have.
In many cases, we reduce the specificity of error messages thrown. For
instance, if an invariant was violated on an edge `e`, then we might
have thrown an error with message `EdgeAddress.toString(e.address)`. But
we did so not because we thought that this was genuinely worth it, but
only because we were forced to explicitly throw an error at all. These
errors should never be hit, anyway, so we don’t feel bad about replacing
these with errors that are simply the string `"null"` or `"undefined"`,
as appropriate.
Test Plan:
Running `yarn travis --full` passes, and the cred explorer still seems
to work with both populated and empty `localStorage`.
wchargin-branch: use-null-util
Summary:
This commit adds a module with four functions: `get`, `orThrow`, `map`,
and `orElse`.
Here is a common pattern wherein `get` is useful:
```js
sortBy(Array.from(map.keys()), (x) => {
const result = map.get(x);
if (result == null) {
throw new Error("Cannot happen");
}
return result.score;
});
// versus
sortBy(Array.from(map.keys()), (x) => NullUtil.get(map.get(x)).score)
```
(The variant `orThrow` allows specifying a custom message that is only
computed in the case where the error will be thrown.)
Here is a common pattern where `map` is useful:
```js
arr.map((x) => {
const result = complicatedComputation(x);
return result == null ? result : processResult(result);
});
// versus
arr.map((x) => NullUtil.map(complicatedComputation(x), processResult))
```
In each of these cases, by using these functions we gain a dose of
safety in addition to our concision: it is tempting to “shorten” the
expression `x == null ? y : z` to simply `x ? y : z`, while forgetting
that the latter behaves incorrectly for `0`, `false`, `""`, and `NaN`.
Similar patterns like `x || defaultValue` also suffer from this problem,
and can now be replaced with `orElse`.
Designed with @decentralion.
Test Plan:
Unit tests included; run `yarn travis`.
wchargin-branch: null-util
There's no sense having a landing page with no content and
a nav bar with only one meaningful options. We can re-add
them later if we actually need navigation
Test plan: Local testing
Summary:
When updating `PagerankTable` to work with contributions, we found it
difficult to keep track of everything when we tried to do two things
simultaneously: compute the values to be displayed, and render them
hierarchically. @decentralion suggested computing the relevant data
ahead of time, and then having a straightforward React component to
render this structure. This would incidentally make `PagerankTable`
easier to test.
This commit implements that data structure and the function to create
it from a `PagerankResult`. A subsequent commit will update
`PagerankTable` accordingly.
As evidence that this structure is well-designed, note that the main
contents of a contribution row can be rendered entirely from a
`ScoredContribution` datum (though the component will still of course
require the full `PagerankNodeDecomposition` to pass down to its
children). (At least, I think that it can be!)
Designed with @decentralion.
Test Plan:
Unit tests added. I have checked that the snapshot is structurally
correct: each node has contributions with the correct contributors.
I did not manually compute the stationary distribution and check the
snapshot for correctness. The snapshot is complemented by automated
tests.
wchargin-branch: pagerank-node-decomposition
Summary:
Now that `MapUtil` provides `toObject`/`fromObject`, we can keep storing
weights in localStorage while representing them as ES6 `Map`s in memory.
Here are some advantages:
- The code is genuinely more typesafe. While writing this,
I accidentally wrote `edgeWeights.get(key)`, where `edgeWeights`
should have been `nodeWeights`. This was caught at compile time, and
would not have been in the previous version.
- Relatedly, the code now has zero `any`-casts as opposed to five.
- The initialization of the default values is not abysmally ugly.
- Whenever we iterate over these maps, (a) we can use `.entries()`,
and (b) we don’t have to cast between string keys and semantic keys.
This simplifies some of the control flow.
- The extra null-checking on `get` forces us to either think about
ways in which the check might fail, or reuse a previously fetched
value that is known to be non-null (perhaps because it came from
`entries`).
- A particularly annoying Prettier line-break is avoided. :-)
Here are some disadvantages:
- The null-pipelining around the `rehydrate` function is a bit
annoying. As @decentralion pointed out, what we want here is not a
default value, but a default value and a function to transform a
present value. This is Haskell’s `maybe : β → (α → β) → Maybe α → β`
or Java’s `optional.map(fromObject).orElse(defaultValue)`. This
commit implements one approach; another is to note that `fromObject`
is invertible, writing `fromObject(LocalStore.get(k, toObject(d)))`.
- That’s it, I think?
Test Plan:
I’ve tested that the sliders for both edge and node weights correctly
influence the PageRank behavior, that the component is properly
initialized with an empty localStorage, and that the component properly
rehydrates from localStorage.
wchargin-branch: weightconfig-maps
Summary:
These call sites were selected from `git grep Map`. In this commit, we
only add usage of the utility functions; we do not change any existing
object types to maps.
Test Plan:
Running `yarn travis --full` passes.
wchargin-branch: use-map-util
Summary:
We’d like to like ES6 `Map`s, because they provide better type safety
than objects (primarily, `Map.prototype.get` has nullable result type).
However, the vanilla APIs are weak. Prominent problems are that `Map`s
always become `"{}"` under `JSON.stringify`, that there is no easy way
to convert between `Map`s and objects, and that there are no functions
to map over the keys and values of `Map`s.
In this commit, we add versions of those functions to a utility module.
The value-level implementations are straightforward, but these functions
nevertheless deserve a utility module because the types are somewhat
tricky to get right. The implementation requires casts through `any`,
and these should be written, analyzed, and proven correct just once. (In
particular, it would be easy to write an unsound type for `fromObject`.)
In a followup commit, we will amend existing portions of the codebase to
use these functions.
Test Plan:
Unit tests added; run `yarn travis`.
wchargin-branch: map-util
This commit adds another bank of sliders to the cred explorer, for
changing the directionality of edges. The sliders have the range [0,1]
with step size of 0.01.
The layout is pretty ugly and clearly should be refactored. But playing
with these sliders is interesting :)
Test plan: We don't have any unit tests on the WeightConfig, but I did
drive it by hand. An interesting experiment is to set the AUTHORS edge
directionality to 1, so that users get no credit for authoring posts. As
expected, this utterly tanks the users' scores; many users then have a
score of -Infinity.
Summary:
If we want to snapshot an edge, then none of the available options is
ideal:
- Snapshotting the edge directly includes literal NUL bytes in the
snapshot file, so it is treated as binary. This is bad.
- Using `edgeToString` works, but all fields of the edge are combined
into a single string, which is somewhat hard to read.
- Using `edgeToParts` works, but each address in the edge takes up a
lot of visual space: one line per part in the address. This is also
somewhat hard to read.
This commit adds `edgeToStrings`, which simply applies the appropriate
`toString` operation to each field of an edge.
Test Plan:
Unit tests added; run `yarn travis`.
wchargin-branch: edge-to-strings
The implementation is similar to the LocalStore usage in
`credExplorer/app.js`. Had to make a spurious refactor from Map to
Object because ES6 maps don't stringify by default, and I didn't feel
like writing a custom JSON serializer.
Test plan:
Didn't add unit tests, although at some point we should come up with a
nice LocalStore mock and test LocalStore code. I did, however, manually
try it out and verify that it works :)
Paired with @wchargin
Summary:
Prettier inserted these in a previous version of the code, but the lines
got shorter and so Prettier no longer minds if we remove the breaks.
Test Plan:
shipitquick
wchargin-branch: remove-line-breaks
Summary:
This change is motivated by the behavior of loops, but applies more
generally to edges. Previously, a loop would induce two contributions
with the same contributor, but possibly different weights: one with the
to-weight of the edge and one with the fro-weight. For one, this is
annoying to downstream clients, who would like to use the contributor as
a superkey. But it is also somewhat strange that a single contributor
could have two different weights.
The same applies to edges in general: every edge induces two
contributions with the same contributor, of type `NEIGHBOR`.
As of this commit, we replace `NEIGHBOR` with `IN_EDGE` and `OUT_EDGE`,
one of each induced by each edge. This has the effect that a contributor
maps to at most one contribution.
Test Plan:
Existing unit tests updated.
wchargin-branch: separate-in-out-edge-contributions
Summary:
When we convert a graph to a Markov chain, each cell in the transition
matrix is a sum of edge weights from the given `src` to the given `dst`,
plus the synthetic self-loop needed for stability. Performing this sum
loses information: given the transition matrix, a client cannot
determine how much a particular edge contributed to the score of a node
without redoing the relevant computations. In this commit, we expose the
structure of these contributions (i.e., edges and synthetic loops).
This changes the API of `graphToMarkovChain.js`, but it does not change
the resulting Markov chains. It also does not change the API of
`pagerank.js`. In particular, clients of `pagerank.js` will not have
access to the contributions structure that we have just created.
Test Plan:
Existing unit tests have been updated to use the new API, and pass
without change. An additional test is added for a newly exposed
function, even though this function is also tested extensively as part
of later downstream tests.
In one snapshot, one value changes from `0.25` to `0.25 + 1.7e-16`. The
other values in the enclosing distribution do not change, so I think
that it is more likely that this is due to floating-point instability
than an actual bug. (I’m not sure where exactly I commuted or associated
an operation, but it’s quite possible that I may have done so). To
compensate, I added an additional check that the values in the
stationary distribution sum to `1.0` within `1e-9` tolerance; this check
passes.
wchargin-branch: expose-contributions
Previously, when expanding a node in the cred explorer, it would display
the neighboring nodes, but not any information about the edges linking
to that node. If the same node was reached by multiple edges, this
information was not communicated to the user.
As of this commit, it now concisely communicates what kind of edge was
connecting the chosen node to its adjacencies. There's a new `edgeVerb`
method that plugin adapters must implement, which gives a
direction-based verb descriptiong of the edge, e.g. "authors" or "is
authored by".
Test plan:
Unit tests added to the PagerankTable tests, and hand inspection.
Paired with @wchargin
Summary:
This commit adds sliders for each node and edge type (hard-coded for
now), and hooks them up to the cred explorer so that re-running PageRank
uses the newly induced edge evaluator.
Paired with @decentralion.
Test Plan:
We will add tests later. We promise! In the meantime, the results that
appear when you drag a slider and re-run PageRank seem appropriate. For
instance, changing the “Git blob” node type from `0.0` to `-10.0`
results in the Git blobs not dominating the whole view.
wchargin-branch: configurable-weights-ui
Summary:
PageRank wants an _edge evaluator_: a function mapping an edge to its
to-weight and fro-weight. This commit provides functions for creating
edge evaluators based on the high-level, coarse notions of node and edge
types. We use [the formulation described in a comment on #476][1].
Paired with @decentralion.
[1]: https://github.com/sourcecred/sourcecred/issues/476#issuecomment-402576435
Test Plan:
None. We will add tests later. We promise!
wchargin-branch: edge-weight-generators
Summary:
As we add sliders for adjusting the PageRank parameters, we trigger a
bunch of unneeded renders on `PagerankTable`. As `PagerankTable` is a
pure component, we can [mark it and its children as such][1] to see notable
performance improvements: the `Array.from` and `sort` in its `render`
method are showing up on the flamegraph.
[1]: https://reactjs.org/docs/react-api.html#reactpurecomponent
Paired with @decentralion.
Test Plan:
Unit tests pass, whereas if we instead implement `shouldComponentUpdate`
by `return false` then the interaction tests fail. Also, `yarn start`
seems to behave as expected as we switch among different graphs.
wchargin-branch: pure-pageranktable
Summary:
To verify that the correct graph is loaded, we display the graph’s node
and edge count in the UI. As previously implemented, this would be
recomputed on every change, requiring iteration over all nodes and edges
of the graph. On my machine (T440s) and the SourceCred graph, this
induced an ~80ms performance floor for any operations that caused the
app to re-render, which is noticeable.
This commit retains the useful information, but computes it only at
graph load time.
Paired with @decentralion.
Test Plan:
Note that the behavior is unchanged.
wchargin-branch: cache-graph-size
Summary:
See #432. This commit switches the GitHub Markdown parser from matching
regular expressions against the raw post body to matching the same
regular expressions against the semantic text of a Markdown document.
See test cases for `parseReferences` and `parseMarkdown` for more
details.
There are no changes to snapshots or cached GitHub data because the
Markdown in the example repository is simple enough to be properly
parsed by a simple approach that treats everything as literal text.
The change to the “finds numeric references in a multiline string” test
case is to strip leading indentation from the string in question. As
previously written, the test had four spaces of leading indentation on
each line, causing the Markdown parser to interpret it as a code block,
in turn causing our logic to (correctly) omit it entirely. The revised
version of the test has no leading indentation.
Test Plan:
Light unit tests included; these tests are intended to verify that the
actual Markdown logic from `parseMarkdown` is being used, and that file
has more extensive tests. Additionally, `yarn travis --full` passes.
wchargin-branch: github-parse-markdown
Summary:
This commit exposes a function of type `(string): string[]` to
encapsulate the whole Markdown pipeline, from parsing to AST
transformation to text node extraction. Clients of this module do not
need to know about `commonmark`.
Test Plan:
A single comprehensive test case has been added.
wchargin-branch: text-blocks
Summary:
The regular expressions used to detect GitHub references were of the
form `/(?:\W|^)(things)(?:\W|$)/gm`, where the outer non-capturing
groups were intended to enforce a word boundary constraint. However,
this caused reference detection in strings like `"#1 #2"` to fail,
because the putative matches would be `"#1 "` and `" #2"`, but these
matches overlap, and the JavaScript RegExp API (like most such APIs)
finds only non-overlapping matches. Therefore, in a string of
space-separated references of the same kind, only every other reference
would be detected.
A solution is to use a positive lookahead instead of the second
non-capturing group: i.e., `/(?:\W|$)/` becomes `/(?=\W|$)/`. (Ideally,
the first non-capturing group would just be a lookbehind, but JavaScript
doesn’t support those.)
In some cases, using `\b` is a simpler solution. But this does not work
in all cases: for instance, it works for repo-numeric references, but
does not work for numeric references, because the presence of the hash
means that there cannot be an immediately preceding word boundary. For
consistency, I opted to use the lookahead solution in all cases, but any
solution that passes tests is okay with me.
Test Plan:
Regression tests added. They fail before this patch and pass with it.
wchargin-branch: fix-space-separated-github-references
Summary:
The `^` metacharacter only matches the start of a non-initial line when
the `m` (“multiline”) flag is set on the RegExp. This flag was set only
for the GitHub URL RegExp.
Test Plan:
Regression tests added. They fail before this patch and pass with it.
wchargin-branch: fix-sol-github-references
Test plan:
`git grep -i v3` only shows incidental hits in longer strings
`yarn travis --full` passes
`yarn backend` works
`yarn build` works
`yarn start` works
`node bin/sourcecred.js start` works
`node bin/sourcecred.js load sourcecred example-github` works
Paired with @wchargin
Summary:
The bridge introduced in #448 has now served its purpose, and may be
deconstructed. This implements the first part of the last step of the
plan described in that pull request.
Paired with @decentralion.
Test Plan:
After `yarn backend && yarn build`:
- `node bin/sourcecred.js start` works, and
- `yarn start` works, and
- `yarn travis --full` works.
wchargin-branch: demolish-bridge
Test plan:
`node bin/sourcecred.js load sourcecred example-github` works
`yarn start` works
`node bin/sourcecred.js start-v3` works
`yarn travis --full` passes
Paired with @wchargin
Summary:
This could also be moved into the bridge directory, but this way is
marginally easier, and it doesn’t really matter in the end.
Test Plan:
`yarn backend` followed by `node bin/sourcecredV3.js start-v3` works.
wchargin-branch: start-v3
The `load` command replaces `plugin-load`. By default, it loads data for
all plugins, and does so in parallel using execDependencyGraph. If
passed the optional `--plugin` flag, then it will load data just for
that plugin.
As an implementation detail, when loading all plugins, load calls itself
with the plugin flag set.
Usage:
`node bin/sourcecred.js load repoOwner repoName`
Test plan:
Tested by hand; I blew away my SourceCred directory and then loaded the
example-github repository.
This integrates the PageRank table from #466 into the v3 cred explorer
app, bringing the v3 frontend to better-than-parity with v1!
Test plan:
Some unit tests were included, and running `yarn start` and inspecting
the App reveals that it is working correctly. Loading a PageRank result
and then changing the repository no longer triggers a crash :).
Paired with @wchargin
Ports #265 to the v3 branch, along with some tweaks:
- Only display log score, and normalize them by adding 10 (so that most
are non-negative)
- Change colors to a soothing green
- Improve display, e.g. make overflowing node description text wrap
within the row
Implements most of the tests requested in #269
Test plan: Many unit tests added
Paired with @wchargin
Summary:
This enables plugins to specify different semantic types of nodes, along
with human-readable names. This will be used, for instance, in the cred
explorer, where users may filter to one of these node types.
Paired with @decentralion.
Test Plan:
Flow passes.
wchargin-branch: plugin-node-types
Summary:
This presents a human-readable name for a plugin. It’s not yet used
anywhere.
Paired with @decentralion.
Test Plan:
Flow passes.
wchargin-branch: plugin-name
Test plan:
Run the following commands:
```
node bin/sourcecredV3.js load-plugin-v3 sourcecred example-github --plugin=git
node bin/sourcecredV3.js load-plugin-v3 sourcecred example-github --plugin=github
yarn start
```
Then, navigate in-browser to the v3 cred explorer and load data for
`sourcecred/example-github`. The following messages are printed to
console:
```
GitHub: Loaded graph: 31 nodes, 73 edges.
Git: Loaded graph: 15 nodes, 19 edges.
Combined: Loaded graph: 44 nodes, 92 edges.
```
Paired with @wchargin
Summary:
This enables grabbing the GitHub relational view from disk and
converting it to a graph on the client.
Paired with @decentralion.
Test Plan:
For testing, information about the GitHub graph is printed when you
click the “Load data” button in the UI. Do so.
wchargin-branch: github-plugin-adapter
Summary:
Text input boxes for repository owner and name now appear. “Loading the
data” consists of logging the attempt to the console.
Test Plan:
Run `yarn start`, and note that the inputs are keyed against the same
local store key as their V1 equivalents. Note that clicking “Load data”
prints a message to the console.
Paired with @decentralion.
wchargin-branch: v3-load-data-ui
In GitHub, you can make cross repo references. For example,
sourcecred/sourcecred#459 is one such reference. This commit adds
support for detecting those references and adding them to the GitHub
graph.
Test plan:
See attached unit tests.
This commit adds `loadGitData`, which clones the git repository for a
given repo and saves the corresponding git graph. It also adds that
method to the `loadPlugin` command, so that the following command now
works:
```
$ node bin/sourcecredV3.js load-plugin-v3 sourcecred example-git --plugin=git
```
After running that command, the correct file is present:
```
$ du -sh tmp/sourcecred/data/sourcecred/example-git/git/graph.json
28K /home/dandelion/tmp/sourcecred/data/sourcecred/example-git/git/graph.json
```
The command takes:
| repository | time (s) |
:------------------------- | ----------:
| `sourcecred/example-git` | 1 |
| `sourcecred/sourcecred` | 5 |
| `ipfs/js-ipfs` | 18 |
| `ipfs/go-ipfs` | ∞ (OOM) |
This module exposes a method, `pagerank`, which is a convenient entry
point for taking a `Graph` and returning a `PagerankResult`. This
obviates the need for `src/v1/app/credExplorer/basicPagerank.js`.
Test plan: Unit tests included.
I'm planning to make a `pagerank.js` module that is a clean entry point
for all the graph-pagerank-related code, so it will be cleaner to expose all
the default options there.
Test plan: travis
Paired with @wchargin
If `findStationaryDistribution` is passed `0` as `maxIterations`, then
it should return the initial distribution.
Test plan: see new unit test
Paired with @wchargin
This commit enables paired authorship on GitHub authored entities.
If the entity has the string "Paired with" in the body, followed by a
username reference, that entity will be recorded as having dual
authorship, with the nominal author and the paired-with author being
treated identically in the relational view and the graph.
If there's a need to pair with more than one author, the "Paired with"
signifier may be repeated. The regex matcher is forgiving of
capitalizing the P or W, and an optional colon may be added immediately
after the word "with".
Note that the code assumes that every `TextContentEntity` is also an
`AuthoredEntity`. If that changes, it will cause a type error and we'll
need to refine the code somewhat.
As implemented, it is impossible for the same user to author a post
multiple time; if this is textually suggested (e.g. by a paired-with
reference to the post's nominal author), the extra paired-with
references are silently ignored. Also, having a paired-with reference
suppresses the basic reference (although it is possible to have a post
that is paired with someone, and additionally references them).
Test plan:
Tests have been updated, and the behavior of the parser is extensively
tested. For an end-to-end demonstration, I've also added a unit test in
the relational view that verifies that sourcecred/example-github#10 has
two authors. You can also see that the graph snapshot has updated to
include additional authorship edges (and that corresponding reference
edges have disappeared).
Closes#218
Summary:
This implements Step 3 of the plan described in #448.
Test Plan:
Run `yarn start` and navigate to `/v3` (by clicking the nav link).
wchargin-branch: branch-v3
Summary:
The bridge now lets you select any version of the app that you want, as
long as that’s V1, because that’s the only version that exists. We’ll
add a V3 version shortly.
This implements Step 2 of the plan described in #448.
Test Plan:
In `yarn start` and `node bin/sourcecred.js start`, note that navigating
to `/` redirects to `/v1`, and that the cred explorer works.
wchargin-branch: bridge-select
Summary:
Our build system doesn’t make it easy to have two separate React
applications, which we would like to have for the V1 and V3 branches.
Instead, we’ll implement a bridge to maintain compatibility.
The plan looks like this:
1. Change the app from pointing to V1 to pointing to a bridge
2. Move the router into the bridge and move the V1 app from the `/`
route to the `/v1` route (e.g., `/v1/explorer`)
3. Add a V3 app under the `/v3` route
4. ???
5. Delete the V1 app and remove it from the bridge
6. Delete the bridge and move the V3 app from the `/v3` route to `/`
This commit implements Step 1.
Test Plan:
To verify that the bridge is in fact showing, apply
```diff
diff --git a/src/bridge/app/index.js b/src/bridge/app/index.js
index 379e289..72e784c 100644
--- a/src/bridge/app/index.js
+++ b/src/bridge/app/index.js
@@ -9,5 +9,11 @@ const root = document.getElementById("root");
if (root == null) {
throw new Error("Unable to find root element!");
}
-ReactDOM.render(<V1App />, root);
+ReactDOM.render(
+ <React.Fragment>
+ <h1>Hello</h1>
+ <V1App />
+ </React.Fragment>,
+ root
+);
registerServiceWorker();
```
and say “hello” back to the app.
wchargin-branch: bridge
Summary:
This provides a command-line entry point `load-plugin-v3` (which will
become `load-plugin` eventually), which fetches the GitHub data via
GraphQL and saves the resulting `RelationalStore` to disk.
A change to the Babel config is needed to prevent runtime errors of the
form `_callee7` is not defined, where `_callee7` is a gensym that is
appears exactly once in the source (in use position, not definition
position). I’m not sure exactly what is causing the error or why this
config change fixes it. But while this patch may be fragile, I don’t
think that it’s likely to subtly break anything, so I’m okay with
pushing it for now and dealing with any resulting breakage as it arises.
Paired with @decentralion.
Test Plan:
Run `yarn backend`, then run something like:
```
node bin/sourcecredV3.js load-plugin-v3 \
sourcecred example-github --plugin github
```
Inspect results in `SOURCECRED_DIR/data/OWNER/NAME/github/view.json`,
where `SOURCECRED_DIR` is `/tmp/sourcecred` by default, and `OWNER` and
`NAME` are the repository owner and name.
This example repository takes about 1.1 seconds to run. The SourceCred
repository takes about 45 seconds.
wchargin-branch: cli-load-plugin
Summary:
Due to oclif’s structure, this entry point shares its `commands`
directory with that of the V1 entry point. We’ll therefore add commands
like `start-v3` as we go.
Test Plan:
`yarn backend` works, and `node bin/sourcecredV3.js start` launches the
V1 server.
wchargin-branch: v3-cli
This adds additions and deletions to the v3 Pull data model, and also
uses them in the pull descriptions.
It's basically a port of #340 to v3.
Test plan: Snapshots
This adds methods for serializing the GitHub RelationalView.
We have not put in the work to ensure that these methods generate
canonical data. Getting the issues in a different order, or finding
references in a different order, can change the JSON output even if the
resulting repositories are equivalent.
@decentralion think it's not worth putting in the effort, since we may
switch to a SQL database soon anyway.
Test plan: travis
Paired with @wchargin
Now that we want to implement RelationalView de/serialization, we need a
way to construct one without adding data to it.
Now that we're allowing `addData` to be called explicitly, we also want
to make sure it's idempotent, which necessitated a small change to
reference handling. A new test verifies idempotency.
Test plan: travis
Paired with @wchargin
Summary:
This makes progress on #432. We’d like to look for GitHub references
only within each text node of the Markdown AST. But there are two
complications:
- Text nodes split across formatting, and it’s valid for someone to
write `*Paired* with @decentralion, but *tested* independently`, or
`**Closes** #12345`, or something.
- Sometimes contiguous blocks of text expand to multiple text nodes,
because of how CommonMark approaches smart punctuation. For
instance: the document `It's got "punctuation" and stuff!` has eight
text nodes ([demo][1]).
In this commit, we introduce functions `deformat` and `coalesceText` to
solve these problems. (They go together because `coalesceText` is useful
for testing `deformat`.)
[1]: https://spec.commonmark.org/dingus/?text=It%27s%20got%20%22punctuation%22%20and%20stuff!
wchargin-branch: markdown-deformat
This commit adds a `description` method that takes a GitHub entity, and
returns a description of that entity. Based on the work in #261.
In contrast to the implementation in #261:
- It won't crash on entities without an author (although we don't have a
test case for this; see #389).
- It handles multi-authors reasonably (although we can't test that, as
we haven't implemented multi-authorship yet; see #218).
Test plan:
Inspect snapshot to see some examples.
Currently, GitHub tests load example data with ad-hoc methods. It makes
it easy for the author of a new test file to forget to clone the test
data (and risk cross-test-file state pollution), or to forget to apply
the correct typing.
This commit factors a shared `example` module which provides a safe way
to access the example data, along with some convenient helpers for
constructing a graph or relational view.
Test plan:
`yarn travis`
Fixes#430.
The Git and GitHub plugins have folders that contain small example data,
as used for tests and snapshots. These folders were called `demoData`
which is misleading since the data isn't used for demos. The folders
themselves contained files called "example", like "example-github.json"
or "exampleRepo.js". Renaming the folders to `example` is cleaner.
Test plan:
`yarn travis --full` passes.
This is a very simple extension of #431 to use the new reference
detection logic added in #429.
Test plan:
Inspect snapshot change for plausibility. Note that the snapshot adds
exactly 16 reference edges, which is the same as the number of
references in the reference snapshot test.
This commit modifies `github/createGraph` to use the `RelationalView`
class created in #424. The code is now much cleaner.
I also fixed some `any`s that were leaking in our test code (due to use
of runtime require for GitHub example data). These anys were discovered
by bumping into uncaught type errors. :)
This commit supersedes #413 and #419.
Test plan:
Observe that the graph snapshot was not changed.
For every `TextContentEntity` (`Issue`, `Pull`, `Review`, `Comment`),
this commit adds a `references` method that iterates over the entities
that the text content entity references.
For every `ReferentEntity` (actually, every entity), this commit adds a
`referencedBy` method which iterates over the text content entities that
reference that referent entity.
This method also adds `referentEntities` and `textContentEntities`
methods to the `RelationalView`, as they are used in both implementation
and test code.
Test plan:
The snapshot tests include every reference, in a format that is very
convenient for inspecting the ground truth on GitHub. For every
reference, it's easy to check that the reference actually exists by
copying the `from` url and pasting it into the browser. I've done this
and they check out. (It's not easy to prove that there are no missing
references, but I'm pretty confident that this code is working.)
Unit tests ensure that the `references` and `referencedBy`
methods are consistent.
Based on offline design discussion with @wchargin, we've decided to
upgrade the `RelationalView` to be *the* comprehensive source for GitHub
data inside SourceCred. The `RelationalView` will contain the full
dataset, including parsed relational information (such as
cross-references between GitHub entities). Then, we will project our
GitHub graph out of the `RelationalView`.
To that end, the `RelationalView` no longer exports raw data blobs.
Instead, it exports nice classes: `Repo`, `Issue`, `Pull`, `Review`,
and `Userlike`. These classes have convenient methods for accessing both
their own data and related entities, e.g. `repo.issues()` yields all
the issues in that repo.
This is effectively a port of #170 into the v3 API. The main difference
is that in v1, the Graph contained this data store, whereas in v3, we
will use this data store to generate the graph.
This supersedes #418.
Test plan:
The snapshot tests are quite readable.
Summary:
This is based on the V1 file `basicPagerank.js`. The API is necessarily
changed for the new graph format, and we export additional utilities
compared to the previous version of the module (useful for testing and
serialization). We also improve the implementation to make it simpler
and easier to understand.
Test Plan:
Unit tests included.
wchargin-branch: v3-graph-markov-chain
Summary:
We’d like to use this test case to generate a Markov chain, which
requires that it not be local to the `graph.js` tests.
Test Plan:
Existing unit tests suffice.
wchargin-branch: expose-advanced-graph
Summary:
This code is independent of the graph abstraction, and so is mostly
copied. The only change is to the structure of the test code (we now
prefer to wrap everything in a big `describe` block with an absolute
path to the module under test).
Test Plan:
Unit tests included.
wchargin-branch: v3-markov-chain
This code is about parsing references out of text, so `parseReferences`
is a better name.
The code that consumes this logic to find all the references in the
GitHub data shall be rightly called `findReferences`
Test plan:
`yarn travis`
Summary:
Closes#417. Submodule commits are dead; long live commits. The ontology
is now:
- A tree includes tree entries.
- A tree entry may have a blob as contents.
- A tree entry may have a tree as contents.
- A tree entry may have a commit as contents.
Test Plan:
Existing unit tests suffice, especially `#commits yields all commits`.
wchargin-branch: git-remove-submodule-commits
Summary:
Submodule commits need not have associated tree objects, in case the
repository to which they belong does not exist in our graph. We’d like
to represent submodule commits as actual commits, which necessitates
this change. See #417 for context.
Test Plan:
Existing unit tests suffice.
wchargin-branch: git-affine-trees
Summary:
This commit adds logic to create the Git graph, modeled after the GitHub
graph creator in #405. In this commit, we do not include the
corresponding porcelain; a Git `GraphView` will be added subsequently.
Kudos to @decentralion for suggesting in #187 that I write the logic to
detect BECOMES edges against the high-level data structures. Due to that
decision, the logic and tests are copied directly from the V1 code
without change, because the high-level data structures are the same. The
new code is exactly the body of the `GraphCreator` class.
Test Plan:
Verify that the new snapshot is likely equivalent to the V1 snapshot,
using the heuristic that the two graphs have the same numbers of nodes
(59) and edges (84). (I have performed this check.)
wchargin-branch: git-v3-create-graph
The `RelationalView` maps the GitHub GraphQL response data into a View
class, which makes it easy to access pieces of GitHub data by their
corresponding `StructuredAddress`.
This will be a valuable companion to the graph, making it possible to
access GitHub node data like the title or body of an issue via the
issue's address. This basically is the supplement to the GitHub graph
that includes the "payloads" from our v1 Graph.
It will also make creating the GitHub graph a lot more convenient,
although I've left that for another commit.
Designed with feedback from @wchargin.
Note: The `RelationalView` objects have a `nominalAuthor` rather than
`author`, so as to distinguish between authorship in the GitHub data
model (entities have at most one author) and in the SourceCred model
(entities may have multiple authors).
Test plan:
Inspect the included snapshots for reasonability, and run unit tests.
The code will be refactored so that references are expressed in terms of
the GitHub node address code; the implementation is copied first so that
the review will be cleaner.
Test plan:
`yarn travis` passes.
Summary:
The public method `checkInvariants` on graph is now cached. The cache is
invalidated when the graph is modified via the public API. As a result
of this change, the time of `yarn ci-test --testPathPattern src/v3/`
decreases from 5.631s to 3.866s (best-of-three timing, but low variance
anyway). This effect becomes much more pronounced as higher-level APIs
check their own invariants by themselves indirectly invoking the graph’s
`checkInvariants` method many times.
Test Plan:
Existing unit tests have been adapted and extended. Tests for the
invariant checking have been updated to call the internal, uncached
method, and new tests have been added to check that the caching behavior
is correct.
wchargin-branch: cache-graph-invariants
This commit:
- adds `github/createGraph.js`
- which ingests GitHub GraphQL response
- and creates a GitHub graph
- adds `github/graphView.js`,
- which takes a Graph
- and validates that all GitHub specific node and edge invariants hold
- every github node may be parsed by `github/node/fromRaw`
- with the right node type
- every github edge may be parsed by `github/edge/fromRaw`
- with the right edge type
- with the right src address prefix
- with the right dst address prefix
- every child node has exactly one parent
- of the right type
- and provides convenient porcelain methods for
- finding repos in the graph
- finding issues of a repo
- finding pulls of a repo
- finding reviews of a pull
- finding comments of a Commentable
- finding authors of Authorables
- finding parent of a ChildAddress
- tests `createGraph`
- via snapshot testing
- by checking the GraphView invariants hold
- tests `graphView`
- by checking individual entities in the example-git repository have
the proper relationships
- by checking that for every class of invariant, errors are thrown if
the invariant is violated
Test plan:
- Extensive unit and snapshot tests added. `yarn travis` passes.
Summary:
This is modeled after the GitHub edge module format. In particular, the
whole length encoding garbage is directly copied. As in that module, we
decline to test the error paths.
Test Plan:
Unit tests added; run `yarn travis`. Snapshots are readable.
wchargin-branch: git-v3-edges
Summary:
This is modeled after the GitHub node module format, with the obvious
alterations plus a bit more type safety in the implementation of `toRaw`
(namely, we check `type` exhaustively).
Test Plan:
Unit tests added; run `yarn travis`.
wchargin-branch: git-v3-nodes
Summary:
Many files are unchanged. Some files have had paths updated, or new
build/test targets added.
The `types.js` file includes payload type definitions. These are
technically independent of the graph abstraction (i.e., nothing from V1
is imported and the code all still works), but it of course implicitly
depends on the V1 model. For now, we include the entirety of this file,
just so that we have a clean copy operation. Subsequent commits will
strip out this extraneous code.
Suggest reviewing with the `--find-copies-harder` argument to Git’s
diffing functions.
Test Plan:
Running `yarn travis --full` passes. Running
./src/v3/plugins/git/demoData/synchronizeToGithub.sh --dry-run
yields “Everything up-to-date”.
wchargin-branch: git-v3-copy
Summary:
We had `edgeExamples`, wherein we constructed examples of edge addresses
(not actual edges) by manual instantiation. We checked that these
matched snapshots, and then we also called `createEdge` a bunch to
create actual edges, and checked that those matched snapshots, too.
Consequently, we had twice as many snapshots as we needed, and also
defined twice as many edge addresses as we needed.
Test Plan:
Note that snapshot contents are either deleted or unchanged.
wchargin-branch: simplify-github-edge-tests
Test Plan:
Observe that the new snapshots are easier to read. Might as well make
sure that they encode the same data as the old snapshots, too. Note that
the backslash character no longer appears in this snapshot file. :-)
wchargin-branch: use-edgeToParts
Summary:
We have `edgeToString`, which formats edges as nicely human-readable
strings. However, these strings have some quotes in them, and so when
they are themselves stringified (e.g., as part of a Jest snapshot), they
become much harder to read. We thus introduce `edgeToParts` to make our
snapshots more readable.
Test Plan:
Unit tests added; run `yarn travis`.
wchargin-branch: add-edgeToParts
Summary:
In #394, we uppercased the constants for GitHub node types. However, we
were using string literals instead of constants in the test cases. These
test cases were supposed to cover every error path, but instead ended up
just covering the “bad type” error path many times.
Any one of the following would have prevented this regression:
1. using string constants instead of literals in the test case;
2. throwing and checking more precise error messages; or
3. being alerted that coverage decreased as a result of the change.
In this commit, we enact the first of these options. I’m open to adding
a coverage bot, but don’t feel strongly about it at this time.
Test Plan:
Running `yarn coverage` now shows 100% coverage for the `nodes.js`
module, whereas previously almost all `throw fail();` lines were
uncovered (and the branch coverage was just 76%).
wchargin-branch: fix-github-node-error-tests
- Switch string constant node and edge types (e.g. "REPO") to exported
consts (eg `export const REPO_TYPE`).
- Add (and internally use) a `_Prefix` psuedomodule which contains
per-type address prefixes
- Test that constructing a StructuredAddress with the wrong type is an
error.
Test plan:
Unit tests pass, snapshots unchanged.
Paired with @wchargin
Summary:
This fixes up all instances of `fromParts([])` that are not in
`address.js` or `address.test.js`.
Paired with @decentralion.
Test Plan:
Running `git grep --name-only -F 'fromParts([])'` yields only the two
modules listed above. Existing unit tests suffice for correctness.
wchargin-branch: use-address-empty
Summary:
This can make invocations of `FooAddress.fromParts([])` a bit more
succinct.
Paired with @decentralion.
Test Plan:
Unit tests added. Run `yarn travis`.
wchargin-branch: address-empty
Similar to #390, we now allow filtering the results from `Graph.edges`
by address prefixes. It's a little more complicated than #390, as we
allow filtering by src, dst, or address.
Test plan:
Unit tests added. `yarn travis` passes.
Paired with @wchargin
Simple API addition to match v1/v2 semantics.
In the future, we can perf optimize this if we switch graph to
store nodes organized by shared prefixes.
Test plan:
Unit tests were added. `yarn travis` passes.
Paired with @wchargin
Summary:
This module includes a raw edge type, a structured edge type, and edge
creation functions that take source and destination and create an edge.
Test Plan:
Unit tests added. These cover all of the successful cases, and none of
the unsuccessful cases. We plan to refactor this code Soon™, and it is
hard to see how to nicely factor the tests without just testing the same
code paths over and over.
wchargin-branch: github-edges
Summary:
First, we rename the module itself from `address` to `nodes`: we’d like
to put the edge functions in a parallel `edges` module instead of
cramping it into this one, so it stands to reason that this one should
be called `nodes`.
We also rename the `GithubAddressT` type to `RawAddress`, so that the
module exports `RawAddress` and `StructuredAddress`. The functions then
have much better natural names of `toRaw` and `fromRaw`.
Test Plan:
Existing unit tests suffice.
wchargin-branch: rename-nodes
One of the slight modifications we've made in v3 is to effect the
following renames (as implemented in #380):
PullRequest -> Pull
PullRequestReview -> Review
PullRequestReviewComment -> ReviewComment
This commit just changes the rest of the github code in v3 to follow the
new convention.
Test plan:
`yarn travis --full` passes.
Summary:
This has two primary benefits:
- Humans can look at this snapshot file to see what’s being queried,
or to manually issue a query.
- When we change the programmatically generated query, we can easily
see what the results are in the GraphQL output. This makes it easy
to verify that a change is correct.
Test Plan:
None.
wchargin-branch: snapshot-query
Summary:
This module exposes a structured type `StructuredAddress`, an embedding
`GithubAddressT` of this type into the `NodeAddress` layer, and
functions to convert between the two.
Paired with @wchargin.
Test Plan:
Unit tests added, with full coverage. Snapshots are easily readable.
This commit copies the following logic necessary for downloading GitHub
data into v3. Minimal changes have been made to accomodate the new path
structure.
Test plan:
- Manually ran plugins/github/fetchGithubRepoTest.sh and verified that
it can correctly pass and fail
- Added the v3 github repo test to `yarn travis --full`
- Ran `yarn travis --full` and it passed
Paired with @wchargin
Summary:
We’ve added a comment directly on a pull request.
Paired with @decentralion.
Test Plan:
`yarn travis --full` passes.
wchargin-branch: update-example-github
Summary:
Each of the invariants listed at the top of the `Graph` class is now
explicitly checked by `checkInvariants`, which is called at the end of
each `Graph` method during tests only. This is powerful: it means that
not only do our tests for `Graph` test the graph, but also any tests
that depend on `Graph`—e.g., plugin code—will give us extra invariant
testing on `Graph`. As noted in a comment, if this becomes bad for
performance, we can blacklist expensive tests or whitelist tests that we
care about.
A graph method may assume that the graph invariants hold before the
method is invoked. Within the body of a graph method, invariants may be
violated, but the method must ensure that the invariants hold
immediately before it returns or yields. A consequence of this is that
if a graph function internally calls a public function (e.g., `addEdge`
might call `hasNode` to check that the source and destination exist),
then it must ensure that the invariants hold before the internal call.
This is not an “implementation detail” or “caveat”; it is simply part of
the interface of public functions. It is legal and reasonable for
private helper functions to explicitly not expect or not guarantee that
particular invariants hold, and in this case the exception should be
documented. (This is not yet the case in any of our code.)
Finally, note that the `checkInvariants` method should not call any
public methods, because those methods in turn call `checkInvariants`. If
this becomes a huge pain, we can look into implementing some kind of
“only check invariants if the invariants are not actively being
checked”, but I’d much rather not do so if we don’t have to.
Test Plan:
Running `yarn coverage` indicates that each of the failure cases is
verified. In principle, I’d be willing to add a test that parses the
source code for `graph.js` and verifies that each `return`, `yield`, or
implicit return is preceded by an invariant check. But I don’t really
want to implement that right now.
wchargin-branch: automatic-invariants
The `advancedGraph` is an example graph defined in `graph.test.js`.
It shows off many tricksy features, like having loop edges, multiple
edges from the same src to same dst, etc. We also provide two ways of
constructing it: `graph1` is straightforward, `graph2` adds tons of
spurious adds, removes, and odd ordering. This way we can ensure that
our functions treat `graph1` and `graph2` equivalently.
Test plan:
New unit tests are added verifying that `equals`, `merge`, and
`to/fromJSON` handle the advanced graph appropriately.
The serialization scheme uses `IndexedEdge`s:
```js
type Integer = number;
type IndexedEdge = {|
Address: EdgeAddressT,
srcIndex: Integer,
dstIndex: Integer,
|}
```
The nodes are first sorted. Then, we generate indexed edges from the
regular edges by replacing each node address with its index in the
sorted order. This encoding reduces the number of addresses serialized
from `n + 3e` to `n + e` (where `n` is the number of nodes and `e` is
the number of edges).
This is based on work in #295, but in contrast to that PR, we do not
index the in-memory representations of graphs. Only the JSON
representation is indexed.
Test plan:
Unit tests added. A snapshot test is also included, both to make it easy
to inspect an example of a JSON-serialized graph, and to ensure
backwards-compatibility. (The snapshot likely should not change
independent of the VERSION string.)
* Implement `Graph.merge`
Tests are mostly copied over from the v2, as implemented in #320.
Some new tests were added, e.g. checking that Merge correctly handles
10 small graphs combined.
Test plan:
See unit tests.
* Reimplement `Graph.copy` using `Graph.merge`
Test plan:
Existing unit tests suffice
Suggested by @wchargin
Tests are mostly copied over from the v2, as implemented in #320.
Some new tests were added, e.g. checking that Merge correctly handles
10 small graphs combined.
Test plan:
See unit tests.
Summary:
The previously listed invariants were weak on two counts. First, it was
unstated that the keys of `_inEdges`, `_outEdges`, and `_nodes` should
coincide. Second, the “exactly once” condition on edge inclusion had the
unintentional effect that edge absent in `_edges` but present twice or
more in each of `_inEdges` and `_outEdges` would not violate the
invariant.
Test Plan:
Stay tuned.
wchargin-branch: strengthen-invariants
The implementation is quite simple. The tests are somewhat more
comprehensive than in v2 or v1. We now test that copies are equal to the
original in a variety of situations.
Test plan:
Unit tests added.
It turns out we forgot to add this to the API, so I added it. I also
implemented it. The tests are pretty thorough; as an added innovation
over our previous tests (e.g. in #312 and #61), we now consistently test
that equality is commutative.
In contrast to our previous implementations, this one is massively
simpler. That's an upside of using primitive ES6 data structures to
store all of the graph's information... which is itself an upside of not
trying to store arbitrary additional information in the graph. Now we
can just do a deep equality check on the underlying nodes set and edges
map!
We might be able to performance tune this method by taking advantage of
the structure of our nodes and edges. This should suffice for now,
though.
Paired with @wchargin
Test plan:
Unit tests were added. Run `yarn travis`
Dandelion Mané
William Chargin
