-
Notifications
You must be signed in to change notification settings - Fork 23
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 base calculator #24
Conversation
a8867c2
to
6c11906
Compare
Added some questions, otherwise lgtm, feel free to merge after addressing. |
src/calculator.ts
Outdated
); | ||
|
||
const contributions: Array<Contribution> = rawContributions.map( | ||
(raw: any) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we type this instead of using any?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a RawContribution interface but it's not changing a lot. We would probably need to throw an exception if a field is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of changes requested! Seems to work as expected!
`${this.chainId}/rounds/${this.roundId}/votes.json` | ||
); | ||
const projects = this.parseJSONFile(`${this.chainId}/projects.json`); | ||
const applications = this.parseJSONFile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON.parse
could throw errors if the file is not found, has incorrect permissions, or has malformed JSON. We should consider handling the a thrown error here.
const augmented: Array<AugmentedResult> = []; | ||
for (const id in results) { | ||
const calc = results[id]; | ||
const project = projects.find((p: any) => p.id === id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its more readable with names such as matchingProject
and matchingApplication
and helps differentiate the augmented data from input data
totalReceived: calc.totalReceived, | ||
sumOfSqrt: calc.sumOfSqrt, | ||
matched: calc.matched, | ||
projectName: project?.metadata?.title, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it good to consider that these values may not exist, but if the project or metadata do not exist - do we want to handle that a particular way?
src/calculator.ts
Outdated
private chainId: string; | ||
private roundId: string; | ||
|
||
constructor(baseDataPath: string, chainId: string, roundId: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While not strictly necessary, I think checking constructor arguments would be a great piece of mind, e.g.
if (!baseDataPath.trim() || !chainId.trim() || !roundId.trim()) {
throw new Error("All parameters must be non-empty and non-blank strings");
}
closes gitcoinco/grants-stack#1288