-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: daily & monthly participants #10
Conversation
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
res, | ||
pgPool, | ||
fetchDailyParticipants) | ||
} else if (req.method === 'GET' && segs[0] === 'participants' && segs[1] === 'monthly' && segs.length === 2) { |
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.
} else if (req.method === 'GET' && segs[0] === 'participants' && segs[1] === 'monthly' && segs.length === 2) { | |
} else if (req.method === 'GET' && pathname === '/participants/monthly') { |
no need to use segments if we have static urls
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.
(also applies above and below)
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.
Yeah, I was wondering about this too.
But then your suggestion changes how we handle paths like /participants//monthly
and thus it would introduce inconsistency with the rest of our API 😢
Maybe I can have a "normalised path string" created via segments.join('/')
and us that.
WDYT?
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.
If it gets complicated like this, let's just switch to fastify 🤷♂️ I don't mind all these edge cases, since I don't yet see any problems arise from them. If you want this to be tighter, I propose let's just make the switch
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.
TBH, I don't have an appetite to spend more time on this. I know the current code could be improved; maybe we can also simplify it by not worrying about inconsistencies in edge case handling. Either way, I feel it's a minor detail, and we have bigger fish to fry.
I am going to land what we have now; I added the "Migrate our REST APIs to Fastify" task to our M4.2 milestone.
@@ -43,6 +47,20 @@ const handler = async (req, res, pgPool) => { | |||
res, | |||
pgPool, | |||
fetchRetrievalSuccessRate) | |||
} else if (req.method === 'GET' && segs[0] === 'participants' && segs[1] === 'daily' && segs.length === 2) { |
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.
} else if (req.method === 'GET' && segs[0] === 'participants' && segs[1] === 'daily' && segs.length === 2) { | |
} else if (req.method === 'GET' && pathname === '/participants/daily') { |
Co-authored-by: Julian Gruber <[email protected]>
Co-authored-by: Julian Gruber <[email protected]>
Expose the new public stats introduced by filecoin-station/spark-evaluate#133
Note: the CI will keep failing until we land the spark-evaluate PR and Dependabot upgrades our repo to use the new spark-evaluate main version.
Links: