* Highlight tableRows on :hover and :focus-within
Resolves#1041
The purpose of this commit is to make the pagerankTable easier
to read, as it's currently difficult to distinguish which score is
associated with which row because of the tight spacing of the
rows and the space between the score column and the row detail column.
@wchargin provided the implementation using `linearGradient()`
and `backgroundImage`s.
A side effect of highlighting the row on `focus-within` is that the rows
will become highlighted when the expand button is clicked, which we
decided was acceptable.
Test plan:
Yarn test passes.
To test the new highlight behavior, visual/manual inspection
passes.
Also added the Aphrodite className to the snapshot
tests. The combination of testing the className + inline style props
should make these tests sensitive to UI changes in the future.
Screenshots:
<img width="939" alt="screenshot 2019-02-17 15 46 34" src="https://user-images.githubusercontent.com/26695477/52918955-332f5280-32cb-11e9-87d3-887c8877116a.png">
<img width="931" alt="screenshot 2019-02-17 15 45 10" src="https://user-images.githubusercontent.com/26695477/52918953-2f9bcb80-32cb-11e9-9356-82c6dccab4ae.png">
* bump CI
Suggested by @decentralion in his review of #1090
Test plan:
yarn test passes. Also verified that the new test case is
failable, if you pass in the wrong array of nodes to `expect()` or
if you mangle the node filter code.
Continuing work on #1020.
Adding an optional parameter to `nodes()` which enables optional
node prefix filtering.
Test plan:
@decentralion suggested on Discord that the tests should verify:
1) the parameter was passed to `_graph` correctly
2) the augmentation logic was applied correctly
The tests I added are identical to the tests in `graph.test`, except
that they verify that the result of `pagerankGraph` matches that of
`graph`. On one hand, this creates a dependence on `graph`,
as these tests don't verify that the filter works correctly, only that
graph has applied the filter and returned the iterator.
However, my prevailing thought is that it isn't `pagerankGraph's` responsibility
to test the behavior of `graph`, and so testing the exact filter results
of `pagerankGraph` like we do in `graph.test` isn't the best strategy, and
testing that `pagerankGraph`'s results equal `graph`'s results is a better strategy.
The tests also check that a `score` is provided alongside each `node` in the iterator,
to minimally satisfy @decentralion's second spec.
yarn test passes.
* PagerankGraph: Add toJSON/fromJSON
This commit adds serialization logic to `PagerankGraph`. As with many
things in PagerankGraph, it's based on the corresponding logic in `Graph`.
Much like graph, it stores data associated with nodes and edges (in this
case, the scores and edge weights) in an ordered array rather than a
map, so as to avoid repetitiously serializing the node and edge
addresses.
Test plan: Unit tests added, and they should be sufficient. Also take a
look at the included snapshot.
Part of ongoing work for #1020.
Adds an equals method for the PagerankGraph. This is really quite
straightforward, the logic is based on the matching logic for
`Graph.equals`.
Tests added.
Test plan: The added tests are comprehensive, and they pass.
As discussed in #1004, we want to be able to package metadata with a
graph's nodes and edges. We can do this much more compactly if we store
the metadata as an array, ordered by the corresponding node/edge
address, rather than storing a map. The disadvantage is that clients
then need to manually sort the graph addresses to deserialize.
This commit adds public methods that allow a client to efficiently
retrieve the sorted addresses from the GraphJSON (where they are already
serialized). This behavior is tested. Note that we appropriately don't
allow clients to peek and directly depend on the exact representation of
GraphJSON, we just promise that sorted address retrieval is possible.
Test plan: Unit tests added (and I verified that breaking the sorting
will fail the test).
* Start work on the PagerankGraph
This commit begins work on the `PagerankGraph` class, as described in
[#1020]. As of this commit, the `PagerankGraph` has basic functionality
like retrieving nodes and edges, and running PageRank. However, it is
missing utility functionality like equality testing and serialization,
and doesn't yet have score decomposition logic.
This was mostly produced during a [live coding session]. Thanks to
@BrianLitwin, @anthrocypher, and @wchargin for participating.
Test plan:
The new code is thoroughly unit tested. Please review the test coverage,
and also the quality of the documentation.
[#1020]: https://github.com/sourcecred/sourcecred/issues/1020
[live coding session]: https://github.com/sourcecred/mission/issues/14
* Improvements from self-review
- Don't allow PRG around empty graph, as there's no way to make it
a valid probability distribution
* Add issue ref in TODOs
This commit modifies `markovChain.findStationaryDistribution` so that
in addition to returning the final distribution, it also reports the
final convergence delta.
This is motivated by the proposed API for the new PagerankGraph (see
[#1020]). Also, I think it makes a nice addition to the test code.
Note that this slightly changes the output from `findStationaryDistribution`,
because we now return the first distribution that is sufficiently converged,
rather than that distribution with one additional Markov action.
Test plan:
Unit tests are updated, and `yarn test` passes.
[#1020]: https://github.com/sourcecred/sourcecred/issues/1020
Thanks to @BrianLitwin for semi-pair-programming it
Thanks to @wchargin for extensive review feedback.
Pull #1080 added in a description field for edge types, but put in a
placeholder message for each actual description. This pull adds in
descriptions for each edge type.
Test plan: `yarn test` passes, and additionally
`git grep 'TODO: Add a description for this edge type'` returns no hits.
Reviewed by @BrianLitwin and @wchargin.
* Enable loading private git repositories
This commit enables loading private repositories, assuming that the user
has ssh-agent configured with keys to allow cloning the private
repository, and has provided a GitHub API token with permissions for the
repository in question.
I have not added automated testing. I don't think a cost-benefit
analysis favors adding such tests at this time:
- This code changes very infrequently, and so is unlikely to break
- If it does break, it will be pretty easy to catch and to fix
- the @sourcecred org is on a free plan, which doesn't allow private
repos, so setting up the test case is a bit of a pain
Test plan: `yarn test --full` passes, so I haven't broken existing Git
clone behavior. Locally, I am able to load private repositories.
* Remove unnecessary process import.
Fixes#1019.
Test plan: Loading the prototype works, as does clicking through to different prototype pages.
“Running `git grep -F '/prototypes/'` returns no results; before this commit, it yielded 2 results.”
This commit #811, allowing users to set the weights of node/edge types to 0.
The WeightSlider now sets the weight to 0 when its dragged to its minimum value.
The logic for converting between weights and sliders has also been made more robust,
and is more thoroughly tested.
In cases where we wanted to set the weight to 0 (e.g. backwards Reaction edges),
the default weight has been changed.
Test plan:
Loading the UI, check that the sliders still work as expected (dragging them changes the displayed weight, dragging to the far left sets weight to 0). Check that the weights are consumed as expected (setting weight for issues to 0 leads to no cred for issues). Check that the weights for backwards reaction edges now have 0 weight. `git grep "TODO(#811)"` returns no hits.
PR #1075 added a new EdgeType, and #1080 added a new field to EdgeTypes.
Both PRs merged and this broke the build.
This very trivial commit fixes the build breakage in a noncontroversial
way (copies the placeholder edge description used for every other edge
over).
Test plan: `yarn test` passes.
There's a bug in #1076 where the Node link at
the bottom of the Readme catches the Node
link reference created earlier in the Readme and
links to 'https://nodejs.org/en/' instead of
'https://github.com/nodejs/node`.
Used @wpank's solution to add the Node link
using a reference text, so we could keep
the word "Node" linked, instead of using "Node.js",
which would suggest the organization instead of
the GitHub project.
Test Plan:
Testing that these links work in the live ReadMe on my
fork, instead of copy/pasting the changed bits into
a Markdown parser.
Updating github example data with support
for 🚀 and 👀 reaction types.
This follows #1068 and @decentralion updating
the archived repo with the new reaction types.
`src/plugins/github/fetchGithubRepoTest.sh -u`
(as @decentralion suggested) updated `example-github.json`
`yarn unit` caught two tests with failing snapshot
tests (`createGraph.test` and `relationalView.test`), so
I updated those with `yarn unit -u`
`yarn test -full` caught a failing snapshot test
at `sharness-full`, resolved by updating the
snapshot in `view.json.gz` with
`UPDATE_SNAPSHOT=1 yarn test --full`.
Thanks to @wchargin for the [explanation] on how
to resolve that issue.
[explanation]: https://github.com/sourcecred/sourcecred/pull/1077#pullrequestreview-196805017
**Test Plan:**
`yarn test --full` is passing.
Additionally, the commands:
```sh
filepath="./sharness/__snapshots__/example-github-load/data/sourcecred/example-github/github/view.json.gz" &&
[ -f "${filepath}" ] && # sanity check
diff -u \
<(git show "HEAD:${filepath}" | gzip -d | jq .) \
<(gzip -dc "${filepath}" | jq .) \
;
```
yields the following output:
```
--- /dev/fd/63 2019-01-27 08:34:15.020387301 -0500
+++ /dev/fd/62 2019-01-27 08:34:15.021095696 -0500
@@ -654,6 +654,22 @@
"subtype": "USER",
"login": "decentralion"
}
+ },
+ {
+ "content": "ROCKET",
+ "user": {
+ "type": "USERLIKE",
+ "subtype": "USER",
+ "login": "decentralion"
+ }
+ },
+ {
+ "content": "EYES",
+ "user": {
+ "type": "USERLIKE",
+ "subtype": "USER",
+ "login": "decentralion"
+ }
}
]
}
```
Again, thanks @wchargin's for providing those commands and accompanying
explanation.
Resolves#1054
Added "ROCKET" and "EYES" to the list of reaction types.
Added "ROCKET" as a valid cred signal, kept "EYES" invisible.
My approach was to use `git git grep THUMBS_UP '*.js'`
and `git grep ThumbsUp '*.js'` to find all the relevant files,
as suggested in #1054
**Test Plan**
1) Inspecting Sourcecred/Mission's UI:
[#13] contains: GOT 🚀 FROM 1 user
@BrianLitwin contains: REACTED 🚀 TO 1 issue
@BrianLitwin contains: REACTED 🚀 TO #13
2) Yarn Test passes
3) `github/edges.test` includes a snapshot test to verify
that we can create an edge using ROCKET
4) @wchargin also noted that:
```sh
diff -u <(git grep -c 'THUMBS_UP' '*.js') <(git grep -c 'ROCKET' '*.js')
diff -u <(git grep -c 'ThumbsUp' '*.js') <(git grep -c 'Rocket' '*.js')
```
passes.
`graphql/mirror.test` now includes "ROCKET" and "EYES" in the example
GithubSchema, but their inclusion has no effect
on any tests.
**Screenshots**
1.
<img width="378" alt="screenshot 2019-01-22 09 02 12" src="https://user-images.githubusercontent.com/26695477/51540428-6c87b600-1e24-11e9-8334-1d9d993dce01.png">
2.
<img width="525" alt="screenshot 2019-01-22 09 02 41" src="https://user-images.githubusercontent.com/26695477/51540472-84f7d080-1e24-11e9-8847-245c0c09ddd6.png">
<br>
Shoutout to [this comment], which saved me an untold amount of head-scratching,
and also @Decentralion's help debugging in the Issue thread.
[#13]: https://github.com/sourcecred/mission/issues/13
[this comment]: e0762303d4/src/plugins/github/graphqlTypes.test.js (L13-L15)
* Add descriptions for NodeTypes
As highlighted by @decentralion in issue #807, we need descriptions for Node and
Edge types in the UI to explain to users what each Node and Edge type does. This
PR modifies the type definition for `NodeType` and adds a `+description: string`
field, then updates all NodeTypes throughout the codebase with descriptions.
Test plan:
Verify that all tests pass and the descriptions makes sense.
This commit adds a new `modificationCount` method to `Graph`, which
exposes's that graph's modification count. This enables clients to write
cached data structures on top of Graph, knowing that they can
programatically detect when the cache has been invalidated.
Test plan: Unit tests have been addded; `yarn test` passes.
This commit is motivated by work on #1020.
There are two kinds of plugin adapters: adapters for doing cred
analysis, called "analysis adapters", and adapters for the cred
explorer, which are confusingly called "app adapters".
This commit decreases the confusion by renaming app adapters to explorer
adapters across the codebase. In a future commit, I will add
documentation to the adapter interfaces so that it is clearer to a
newcomer to the codebase why these interfaces exist.
Thanks to @BrianLitwin, who asked a question during [office hours]
that surfaced this issue.
[office hours]: https://github.com/sourcecred/mission/issues/12
Test plan: `yarn test` passes, suggests that this rename went off
without a hitch. Code review as a sanity check.
Also: grepping for `AppAdapter` returns 0 results:
```
$ git grep AppAdapter | wc -l
0
```
Note: After producing this commit, I can confirm that the word "adapter"
starts to look like utter gibberish after you type it often enough.
This commit adds some docstrings for the concepts of NodeType and
EdgeType. I also swapped the order so that `NodeType` comes first,
which is more consistent with usage across the codebase.
This commit makes no changes to the actual code; the only effects
are re-organization and documentation.
Test plan: `yarn test` && human inspection
In response to SourceCred/Mission#15, we switched the default new contributor label from "Contributions Welcome" to "good first issue". This updates the README to reflect that change.
Test Plan: make sure the link redirects to our Issues page filtered for "good first issue" labels.
Summary:
We use Aphrodite, not CSS imports, for styling. We do have a small
`index.css` file that is included during server-side rendering, and is
only referenced from `src/homepage/server.js`. But our `index.js` file
also has a superfluous `import "./style.css"`, which might suggest that
we support CSS imports more generally. This patch removes that import.
Thanks to @brianlitwin on Discord for pointing out that this might be
confusing.
Test Plan:
Verified that, under both `yarn start` and `yarn build`, the appearance
is the same, and the document still includes a `<style>` element with
the contents of `index.css` (which is included by `server.js`).
wchargin-branch: remove-css-import
* Add documentation to the Graph module
This commit adds a module-level docstring that gives an overview of the
Graph class and its importance to SourceCred, as well as adding
docstrings to specific methods.
Test plan:
In addition to review by the SourceCred maintainers, this should be
reviewed by at least one person who is not familiar with the codebase,
so that we can verify that it's actually working as documentation. :)
* Incorporate @wchargin's many suggestions.
Test plan: Human review.
Currently, our underlying test script uses npm rather than yarn to
execute the tests. This is awkward, because we use yarn everywhere else
in lieu of npm. It turns out that some setups have node available
without npm, and in such environments our tests fail with a cryptic
ENOENT error.
This changes the tests to use yarn instead.
Test plan: `yarn test --full` passes.
Thanks to @wpank for help uncovering this issue.
This commit adds a new runOption for execDependencyGraph, namely
`printVerboseResults`. If this flag is true, then execDependencyGraph
will print a "Full Results" section along with the standard error and
standard out of every task, regardless of whether it failed or
succeeded. (Note, this is the existing behavior for all invocations
prior to this commit).
If the flag is not true, then execDependencyGraph will not print a full
results section, and stdout/stderr will be logged only for tasks that
fail.
This commit also modifies `yarn test` to use the new flag so that it
prints verbose tests only when the `--full` option is provided. This is
consistent with our sharness behavior: we print the full sharness logs
only when `--full` was provided.
This fixes#1035, and ensures that running `yarn test` has a high signal
to noise ratio (i.e. it only shows an enumeration of top level tasks).
This improves the developer ergonomics of SourceCred by not having a
super commonly used and core script spam the user with mostly irrelevant
information.
Test plan:
Run `yarn test` when all tests are passing, and observe that the output
has much less noise:
```
yarn run v1.12.3
$ node ./config/test.js
tmpdir for backend output: /tmp/sourcecred-test-6337SZ9smvWsWvqE
Starting tasks
GO ensure-flow-typing
GO check-stopships
GO check-pretty
GO lint
GO flow
GO unit
GO backend
PASS check-stopships
PASS ensure-flow-typing
PASS flow
PASS backend
GO sharness
PASS sharness
PASS check-pretty
PASS lint
PASS unit
Overview
Final result: SUCCESS
Done in 11.66s.
```
Run `yarn test` when there is a real failure (e.g. a unit test failure)
and observe that full details on the failure, including the output from
stdout/stderr, is still provided.
Run `yarn test --full` and observe that full, verbose logs are provided.
As described in #1033: Currently, developers in environments without gnu
coreutils (notably: macOS) get an extremely confusing error message when
they run `yarn test`. This commit ensures that such users instead get a
helpful message with a link to the fix.
Follows @wchargin's proposed approach for fixing #1033, so that Mac
developers get clear guidance on how to get their development
environment working properly.
Expected behavior is that `yarn test` passes when GNU coreutils are
present, and fails with the following message when they are not:
Fixes#1033
```
FAIL check-gnu-coreutils
Exit code: 1
Contents of stdout:
/home/dandelion/git/sc/sourcecred
Contents of stderr:
Error: Your environment does not provide GNU coreutils
You're likely developing on macOS.
Please see the following link for a fix:
https://github.com/sourcecred/sourcecred/issues/698#issuecomment-417202213
```
Test plan:
Verify that on a machine with gnu coreutils present, the command passes
and does not print irrelevant output.
Verify that on a machine without gnu coreutils available, the command
fails, and prints exactly the error message expected, without any extra
output related to the test's implementation.
Paired-with: @anthrocypher
Resolves#1027
Using `repoId.owner/repoId.name` for the project title
because that is how projects are identified on `PrototypePage`.
Created a `<ProjectDetail />` component inside `<App />` that consumes a `RepoId`
and renders a title.
**Test Plan:**
Added two unit tests:
The first verifies that the parent `<App />` component
instantiates a `<ProjectDetai />` component with the correct props.
The current correct prop is a `RepoId` object.
The second test verifies that the `<ProjectDetail />` component renders
the title correctly given the `RepoId`, ie as a `<p>` element
with `repoId.owner/repoId.name` for text.
Visual tests verify that the title is above the Analyze Cred
button, and that clicking from one project to another renders
the appropriate title for separate projects.
Attaching a screenshot as a comment at #1032
for reference:
<img width="1253" alt="screenshot 2019-01-04 13 40 03" src="https://user-images.githubusercontent.com/26695477/50706562-34aeff00-102c-11e9-9c1c-6c1e3fa6c415.png">
This moves the invariant checking code from the top of the Graph class
to the bottom. Most readers of this file will probably be more
interested in seeing the API, and reading the invariant checker first
is likely to be confusing and off-putting.
Test plan: `yarn test` suffices. No semantic change.
This commit substantially improves SourceCred's performance in
production.
Measurement methodology: I create a new tab in Chrome, navigate to my local
prototypes, and select go-ipfs. I then turn on profiling, and click the
analyze button, and then turn off profiling when analysis is done. I
then go to the "bottom-up" tab in the JS analysis box on the bottom and
sort by "Total Time".
__Before this commit:__
| fn | total time | time as % |
|:---------------- | ----------:| ---------:|
| assertValid | 815ms | 8.6% |
| assertValidParts | 261ms | 2.7% |
__After this commit:__
| fn | total time | time as % |
|:---------------- | ----------:| ---------:|
| assertValid | 21ms | 0.2% |
| assertValidParts | 23ms | 0.3% |
Test plan: `yarn test`, also performance measurement as described above.
Fixes#1011.
This adds a blacklisted id for @greenkeeper, a bot which used to be a
user. This is a temporary fix until we solve #998.
Test plan: `yarn test` passes. Before this commit, attempting to load
`probot/probot` fails. After this commit, it succeeds.
When I implemented this function, I incorrectly assumed that
`lodash.sortBy` only calls subsequent accessor functions if there is a
tie from the first accessor. Actually, it calls it every time. We can
avoid lots of wasteful JSON.serialization by just grabbing the exact
properties of interest.
Test plan:
For correctness: `yarn test` suffices, as this functionality is already
tested.
For performance improvement: I ran the full load+analyze workflow, in
Chrome, on twbs/bootstrap. Before this change, decompose took 6.9s;
after this change, it takes 1.3s, for a 5.3x speedup.
Close#943.
Summary:
Our registry was defined to simply be a list of IDs. This is
insufficiently flexible; we want to be able to annotate these IDs with,
e.g., last-updated times (#989). This commit wraps the entries in a
simple object, updating clients appropriately.
Test Plan:
- Run `node ./bin/sourcecred.js load sourcecred/example-github` with a
repository registry in the old format, and note that it errors
appropriately.
- Run `yarn build` with a repository registry in the old format, and
note that it errors (“Compat mismatch”).
- Delete the old registry and re-run the `load` command. Note that it
runs successfully and outputs a registry. Run `yarn build`; note
that this works.
- Load data for two repositories. Run `yarn start`. Note that the list
of prototypes still works, and that you can navigate to and render
attributions for individual project pages.
- Verify that `yarn test --full` passes.
wchargin-branch: repo-id-registry-metadata
Summary:
There have been some breaking changes that require new type annotations,
which is a good thing: these prevent `any`-leakage.
Test Plan:
Run `yarn flow`.
wchargin-branch: flow-v0.86.0
Summary:
This serves as a regression test for #1000.
Test Plan:
Note that `yarn unit` passes with this patch but fails if the change to
the code is reverted, or if the patch in #1000 is reverted. Note that
`yarn build` also passes but fails if the patch in #1000 is reverted.
Note also that `yarn test --full` passes.
wchargin-branch: link-verify-trailing-slash
Summary:
Prior to this commit, clicking the in-copy link to the prototypes page
would raise a console error:
> Warning: [react-router] Location "/prototype" did not match any routes
Test Plan:
Run `yarn start` and click the link.
wchargin-branch: site-fix-homepage-prototype-link
Summary:
Prior to this commit, the prototypes page, which lists just a handful of
repositories, was rendered with a vertical scrollbar: you had to scroll
200px to see the version info. This is silly.
The `height: 100%` is necessary not to get it to fill up the whole page,
but to get it to _not_ fill up ~30 extra pixels. I have no idea why.
Test Plan:
Run `yarn start` and note that `/prototypes/` now renders without a
scrollbar, and with the version info in the bottom-right corner.
wchargin-branch: site-fix-prototypes-page-dimensions
This resolves an outstanding TODO in pagerankNodeDecomposition to remove
the unused sourceScore field.
I have removed it, and it was indeed unused.
Test plan: `yarn test` passes.
Summary:
The `MapUtil` map–object conversion functions used inexact objects for
both input and output. They are in fact stronger than that: they can
accept arbitrary inexact objects and return arbitrary exact outputs.
(Recall that exact objects are subtypes of their inexact counterparts,
so this is the maximally permissive combination.)
Test Plan:
Unit tests added. The “can return an exact object” test fails Flow
before this change. The other tests would have passed already.
wchargin-branch: maputil-exact-output
RepoIdRegistry is used across the project, but not in the explorer. So
it makes very little sense that it live in the explorer module. It's now
moved to core.
Test plan: `yarn test --full` passes
We added a configurable cred feedback url on the theory that we would
create a separate discourse post to collect feedback for each project in
particular.
We've now realized that no one is using this, so it's just vestigial
complexity now. So I'm removing the logic for configuring the feedback
url on a per-project basis.
Instead, we will always link to a Google form for collecting feedback.
Test plan: `yarn test --full` passes, and I manually checked the links.
Historically, a single cred explorer instance could load many different
repositories. This turned out to be an anti-feature: we'd rather have a
particular url hardlink to exploring the cred for a particular project.
This commit removes the repository select from the explorer, and instead
mandates that the explorer always has the RepoId passed down from above.
Besides providing a better UX, this also greatly simplifies the logic
for the explorer, since we no longer have an "initializing state" that
doesn't have any RepoId.
This builds on the work in #984, and swaps out the old "prototype" page
(which has been rendered non-functional by this change) for the new
"prototypes" page. Note that it stays at the same route, so links to
sourcecred.io/prototype will continue to function.
Test plan: Ran `yarn test --full`, and verified that `yarn start`
produces a working site.
Test Plan:
Apply the following patch:
```diff
diff --git a/src/homepage/routeData.js b/src/homepage/routeData.js
index 32d3eb65..aac7fc9a 100644
--- a/src/homepage/routeData.js
+++ b/src/homepage/routeData.js
@@ -38,7 +38,10 @@ const routeData /*: $ReadOnlyArray<RouteDatum> */ = [
path: "/prototypes/",
contents: {
type: "PAGE",
- component: () => require("./PrototypesPage").default([]),
+ component: () =>
+ require("./PrototypesPage").default([
+ {owner: "sourcecred", name: "example-github"},
+ ]),
},
title: "SourceCred prototypes",
navTitle: null, // for now
```
Then, load <http://localhost:8080/prototypes/> and see that there is an
entry in the list, and that it links to
<http://localhost:8080/prototypes/sourcecred/example-github/>. Note that
clicking the link raises a console error because there is no such route.
wchargin-branch: homepage-prototypes-page
Brian Litwin
Dandelion Mané
expravit
Ian Darrow
Ana Noemi
Aseem Sood
William Chargin
