"Explore(r)" does not accurately convey the current state of the
project. In order to more accurately convey the current state,
"Explore(r)" has been updated to "Prototype"
Addresses #584
Test plan: Visual inspection and manual testing of pathing
Currently, the navbar lacks styling, and only provides links to the home and explorer pages.
This change adds:
* Better styling for the navbar
* External links to SourceCred github and twitter
Test plan: visual inspection
Summary:
This subtree has no effect on the new build process; it contains only
stale code.
Test Plan:
Running `yarn test --full` passes. Running `yarn build` and running an
HTTP server on the result indicates the expected behavior, as does
running `yarn start`. A quick `git grep public` finds no amok results.
wchargin-branch: remove-public
This commit removes some unecessary text and whitespace from the cred
explorer. The result is more minimal. It's not super pretty, but that
will come later.
Test plan: Visual inspection
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.
Claire L
William Chargin
Dandelion Mané
