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

Refactor platforms #34

Merged
merged 10 commits into from
Mar 31, 2022
Merged

Refactor platforms #34

merged 10 commits into from
Mar 31, 2022

Conversation

berekuk
Copy link
Collaborator

@berekuk berekuk commented Mar 29, 2022

I did a few things here.

The key change is that all files in backend/platforms/ now conform to the Platform interface, which looks like this:

// fetcher should return null if platform failed to fetch forecasts for some reason
export type PlatformFetcher = () => Promise<Forecast[] | null>;

export interface Platform {
  name: string;
  fetcher: PlatformFetcher;
}

Note that fetcher doesn't store anything in the DB, it just returns the forecasts. And then processPlatform(platform) from backend/platforms/index.ts upserts it.

This way we can:

  • avoid copy-paste
  • do other stuff with fetchers other than update the DB (so fetchers are more pure in their side effects)
  • standardize on the Platform interface so that we can evolve it in the future, e.g. add "longName" field, incremental updates, etc.

I also:

  • renamed all-platforms.ts to index.ts; since it's the "main entry point to platforms", it makes sense to keep it in index
  • removed platforms/all/platformFetchers.ts and platforms/all/platformNames.ts, they are not needed now
  • removed -fetch suffix from platform files
  • added a Forecast type (see platforms/index.ts) which is quite important for future type safety

This is the first step to my umbrella scheduler/platforms layer refactoring plan which I'll describe in a separate issue, and which adds more context about why I'm moving in this direction.

@NunoSempere, please let me know if you dislike any of this! One thing that this PR breaks is your pattern of "call a platform code by uncommenting a // polymarket(); line. But the same can be done with CLI, I think, so nothing significant is lost.

There are also a few minor details we could discuss, e.g. I know I could avoid an explicit name field in a Platform object and detect it with fun.name as it was done before; or at least use export default to avoid mentioning the platform name. There are some downsides to other approaches, though, some of them are too fragile to typos, and some are not compatible with typescript (e.g., it's impossible to specify a type when doing export default without creating an extra variable; also, export default is subtly worse than non-default exports, I believe).

@netlify
Copy link

netlify bot commented Mar 29, 2022

Deploy Preview for metaforecast ready!

Name Link
🔨 Latest commit 64d612a
🔍 Latest deploy log https://app.netlify.com/sites/metaforecast/deploys/62448fa0e2a681000833fd41
😎 Deploy Preview https://deploy-preview-34--metaforecast.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@berekuk
Copy link
Collaborator Author

berekuk commented Mar 29, 2022

(Forgot to push my last commit here which I described in the text above. Fixed.)

@NunoSempere
Copy link
Collaborator

Nice, looks good

@NunoSempere
Copy link
Collaborator

NunoSempere commented Mar 29, 2022

Mmh, actually, there are some merge conflicts, could you look into those? @berekuk. Probably a result of my refactoring polymarket's fetcher to take less memory yesterday.

@berekuk
Copy link
Collaborator Author

berekuk commented Mar 29, 2022

Oh right, I haven't merged your code yet. Just wanted to finish with #36 and other umbrella tasks, so pushed a bit early. Will fix soon.

@berekuk
Copy link
Collaborator Author

berekuk commented Mar 29, 2022

Merged.

I've also discovered that I broke down fun.name calls with anon callbacks, so I took the opportunity to rewrite CLI.

  • jobs registry is now in backend/flow/jobs.ts, with unified "name, description & function" approach
  • cli script code is simplified
  • cli uses names instead of numbers
  • added some color :)

Copy link
Collaborator

@NunoSempere NunoSempere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, though I left a few comments.

src/backend/platforms/index.ts Show resolved Hide resolved
src/backend/platforms/rootclaim.ts Show resolved Hide resolved
@berekuk
Copy link
Collaborator Author

berekuk commented Mar 30, 2022

Oh, I broke givewell and xrisk with this, since mergeEverything takes the data from the platforms list, and the list is missing those two. Don't merge this branch yet.

@berekuk
Copy link
Collaborator Author

berekuk commented Mar 30, 2022

Oh, I broke givewell and xrisk with this, since mergeEverything takes the data from the platforms list, and the list is missing those two.

But it's probably not the reason xrisk is missing from prod. Not sure what's up with that, updating manually, will comment on #39 soon.

@berekuk
Copy link
Collaborator Author

berekuk commented Mar 30, 2022

Oh, I broke givewell and xrisk with this, since mergeEverything takes the data from the platforms list, and the list is missing those two.

Fixed: I added xrisk and givewellopenphil platform descriptions.

Their fetchers are now responsible for reading input/filename.json instead of the old manualSendToDb.ts script, which I removed. I disabled the "read file and return" code in fetchers with early return, so it won't get executed every day in scheduler.

@berekuk
Copy link
Collaborator Author

berekuk commented Mar 30, 2022

Also: turns out I somewhat broke algolia updates in one of my previous PRs.

Here's how it happened:

  • CLI script wasn't finishing properly due to node waiting for unfinished asyncs, and I couldn't find an easy way to check which async is guilty
  • so I just threw in process.exit(); in commandLineUtility
  • but rebuildAlgoliaDatabaseTheEasyWay wasn't awaiting for index.replaceAllObjects, and the npm run cli algolia exited before doing anything useful

I fixed this on this branch in 49d8140, and I'm not sure if this affected prod. Probably not, since in the scheduler job there are a few more steps after algolia, so algolia job had enough time to finish anyway.

Also, I moved algolia settings (app id and search key) to .env (NEXT_PUBLIC_ALGOLIA_SEARCH_KEY and NEXT_PUBLIC_ALGOLIA_APP_ID; already set up on Netlify to avoid issues with deployment).

@@ -38,7 +38,7 @@ export const platforms: Platform[] = [
fantasyscotus,
foretold,
goodjudgment,
goodjudgmentopen,
goodjudmentopen, // note the typo! current table name is without `g`, `goodjudmentopen`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should really be fixed.

@NunoSempere NunoSempere merged commit 9fc8a7d into master Mar 31, 2022
@berekuk berekuk deleted the refactor-platforms branch March 31, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants