Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `sourcecred discourse` command #1374

Merged
merged 1 commit into from Sep 19, 2019

Conversation

@decentralion
Copy link
Member

commented Sep 11, 2019

This adds a new command, discourse, which makes it convenient to load
Discourse servers as standalone SourceCred projects.

For example, you could load the official SourceCred discourse via the
following:

export SOURCECRED_DISCOURSE_KEY=....
yarn backend
node bin/sourcecred.js discourse https://discourse.sourcecred.io credbot
yarn start

I've updated the README with instructions for using the plugin.

Test plan: No automated testing because I see this tool as a temporary
placeholder until we get the SourceCred instances setup. I manually
tested the error cases (e.g. providing an invalid server url) as well as
success cases like the one above.

@decentralion decentralion requested review from Beanow and vsoch Sep 11, 2019

@@ -39,6 +41,8 @@ function usage(print: (string) => void): void {
Commands:
load load repository data into SourceCred
clear clear SoucrceCred data
scores print SourceCred scores to stdout

This comment has been minimized.

Copy link
@decentralion

decentralion Sep 11, 2019

Author Member

(added scores as it was missing)

discourseServer: {serverUrl, apiUsername},
};
const taskReporter = new LoggingTaskReporter();
const params = {alpha: 0.05, intervalDecay: 0.5, weights: defaultWeights()};

This comment has been minimized.

Copy link
@Beanow

Beanow Sep 11, 2019

Member

Looks like --weights support is not yet implemented here and we have duplicated hard-coded values for alpha and intervalDecay.

Suggest extracting the weight file loading function as a common CLI function to add it here. And moving the alpha and intervalDecay values to a constants file or functions, like defaultWeights().

This comment has been minimized.

Copy link
@decentralion

decentralion Sep 12, 2019

Author Member

Resolved in #1376 and #1377. Should be ready for approval. :)

@wchargin

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Why isn’t this done as part of sourcecred load?

@decentralion

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

Why isn’t this done as part of sourcecred load?

I considered refactoring sourcecred load so that it takes an arg structure like:

  • first positional arg is the project id
  • --github foo/bar loads a github repo
  • --github @foo loads an org
  • --discourse-url https://... --discourse-user credbot loads a Discourse instance

This would have an advantage over this sourcered discourse command in that it enables mixing discourse and GitHub projects. However, it is still a short-term temporary solution (I want to migrate to a config-based approach rather than flags) and it breaks existing users. I figured there was no point breaking existing users just to put in a new temporary solution; I'd rather do a single break once we roll out the new instance system. At the same time, the general-purpose project config approach that I'm going to use for SourceCred's own needs (mixed Discourse+GitHub scope, along with node collapsing) will have a very unpolished UI initially, and I want to make it possible for users to try SourceCred on Discourse easily, while I think the GitHub/Discourse hybrid will be a power user (i.e. me) feature for a while.

Therefore, I felt that adding a new command that makes it easy to try out the Discourse plugin, without breaking any existing users, would be the best approach.\

@wchargin

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

SGTM

decentralion added a commit that referenced this pull request Sep 12, 2019
factor loadWeights into Common
As suggested by @Beanow in [a review comment][1], this commit factors
loading weights from disk into a cli/common utility method.

The actual method is really generic, and we have a number of similar
constructions across the codebase (grep for `JSON.parse` to find them).
I considered factoring out a generic utility for loading and
deserializing JSON data from disk in general, but it didn't seem
valuable enough at this time.

Test plan: Unit tests added, existing tests pass.

[1]: #1374 (comment)
decentralion added a commit that referenced this pull request Sep 12, 2019
factor loadWeights into Common
As suggested by @Beanow in [a review comment][1], this commit factors
loading weights from disk into a cli/common utility method.

The actual method is really generic, and we have a number of similar
constructions across the codebase (grep for `JSON.parse` to find them).
I considered factoring out a generic utility for loading and
deserializing JSON data from disk in general, but it didn't seem
valuable enough at this time.

Test plan: Unit tests added, existing tests pass.

[1]: #1374 (comment)

@decentralion decentralion force-pushed the discourse-cli branch from 67f03db to a7f41f5 Sep 12, 2019

@decentralion decentralion changed the base branch from master to common-load-weights Sep 12, 2019

@decentralion decentralion force-pushed the discourse-cli branch from a7f41f5 to 569a53d Sep 12, 2019

@@ -105,11 +104,10 @@ const loadCommand: Command = async (args, std) => {
const projects = await Promise.all(
projectSpecs.map((s) => specToProject(s, githubToken))
);
const params = partialParams({weights});
const plugins = DEFAULT_PLUGINS;
const optionses = projects.map((project) => ({

This comment has been minimized.

Copy link
@Beanow

Beanow Sep 12, 2019

Member

Why is this optionses?

This comment has been minimized.

Copy link
@decentralion

decentralion Sep 12, 2019

Author Member

Because as currently written, load can concurrently load many projects. SO we have load options for each project, so optionses.
Note: I reverted the changes to this file as they weren't needed, and were causing (spurious) test failure.

@decentralion decentralion force-pushed the discourse-cli branch from 569a53d to 4b6430f Sep 12, 2019

decentralion added a commit that referenced this pull request Sep 12, 2019
factor loadWeights into Common (#1377)
As suggested by @Beanow in [a review comment][1], this commit factors
loading weights from disk into a cli/common utility method.

The actual method is really generic, and we have a number of similar
constructions across the codebase (grep for `JSON.parse` to find them).
I considered factoring out a generic utility for loading and
deserializing JSON data from disk in general, but it didn't seem
valuable enough at this time.

Test plan: Unit tests added, existing tests pass.

[1]: #1374 (comment)

@decentralion decentralion changed the base branch from common-load-weights to master Sep 12, 2019

@decentralion decentralion force-pushed the discourse-cli branch from 4b6430f to 2c53d0e Sep 12, 2019

Add `sourcecred discourse` command
This adds a new command, `discourse`, which makes it convenient to load
Discourse servers as standalone SourceCred projects.

For example, you could load the official SourceCred discourse via the
following:

```sh
export SOURCECRED_DISCOURSE_KEY=....
yarn backend
node bin/sourcecred.js discourse https://discourse.sourcecred.io credbot
yarn start
```

I've updated the README with instructions for using the plugin.

Test plan: No automated testing because I see this tool as a temporary
placeholder until we get the SourceCred instances setup. I manually
tested the error cases (e.g. providing an invalid server url) as well as
success cases like the one above. I validated that the weights file
argument is being interpreted correctly (i.e. trying to load invalid
weights produces an expected error message, loading valid weights
results in those weights being present in the UI).

@decentralion decentralion force-pushed the discourse-cli branch from 2c53d0e to 85aa972 Sep 19, 2019

@decentralion decentralion merged commit 007568d into master Sep 19, 2019

2 checks passed

ci/circleci: publish-1 Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@decentralion decentralion deleted the discourse-cli branch Sep 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.