-
Notifications
You must be signed in to change notification settings - Fork 7
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
Pretalx support #190
Pretalx support #190
Conversation
This seems pretty sane to me - happy to approve as-is to keep things moving or should I wait until the checklist is complete - those could possibly be done in follow-up PRs? |
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.
Bar the two tiny nit questions LGTM!
@@ -206,7 +206,7 @@ export class Scheduler { | |||
// Rationale: Sometimes schedules get changed at short notice, so we try our best to accommodate that. | |||
// Rationale for adding 1 minute: so we don't cut it too close to the wire; whilst processing the refresh, | |||
// time may slip forward. | |||
await this.conference.backend.refreshShortTerm((minVar + 1) * 60); | |||
await this.conference.backend.refreshShortTerm?.((minVar + 1) * 60); |
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.
Would it make sense to alert the end-user here if there is no refreshShortTerm
function? (Not sure how likely it is that the lack of this will result in an out-of-date schedule)
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 for this section it's okay. This part runs every 8s to check for new tasks, and optionally refreshes it's cache of the conference when doing so. It's probably not a good idea to tell the user every time, and it's more of a technical detail imo.
for (const pEvent of arrayLike(pRoom.event)) { | ||
if (!pEvent) continue; | ||
// Prefer code, which maps to pretalx. If not available then fall back to @_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.
Is this comment still relevant - I think it might not be?
Fixes #189
This is now based upon the FOSDEM 2024 schema, and correctly parses the production conference data. I chose in the end to use the public JSON schedule format combined with a handful of calls to the API to populate the speaker information. This now mostly works, and is missing: