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

fix: Reformat catalogue builder (+ add ISEB) #1119

Merged
merged 12 commits into from
Feb 4, 2025

Conversation

xgraceyan
Copy link
Contributor

Summary

  • Reformatted catalogue_builder.js into catalogueBuilder.ts
  • catalogueBuilder builds locations.ts alongside buildingCatalogue.ts
  • Replaced locations.ts and buildingCatalogue.ts with its recently updated version
  • Moved catalogueBuilder.ts, buildingCatalogue.ts, locations.ts into apps/antalmanac/src/lib/locations

Important note/potential issue

Currently, the data in buildingCatalogue.ts has its building ID keys as strings instead of numbers. This is because json requires all keys be strings, and JSON.stringify is used in order to write the scraped data to buildingCatalogue.

I can't find a workaround that writes to the file without using JSON.stringify as of right now, so I've just left the IDs as strings for now. To my knowledge, you can still use numbers to index the data, and locations are still able to be fetched properly from other scripts. However, this is still pretty important to note down, and might need another solution soon.

Test Plan

  • Run locations.test.ts and make sure all tests pass (all tests passed when I ran it on my end)
  • Ensure locations are able to be looked up on the map
  • Ensure location name, image, directions, and info are displayed when searched on map
  • Ensure any "missing" locations are present -- ie, ISEB and any others
  • Ensure locations are able to be fetched when clicked from a class lookup
  • Ensure locations are able to be saved properly when adding a custom event
  • Ensure location abbreviations are displayed on the calendar (and wherever applicable)

Issues

Closes #1085

Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

Great improvement to our system! Just some minor suggestions, but overall very thoroughly put together!

apps/antalmanac/src/lib/locations/catalogueBuilder.ts Outdated Show resolved Hide resolved
apps/antalmanac/src/lib/locations/catalogueBuilder.ts Outdated Show resolved Hide resolved
apps/antalmanac/package.json Outdated Show resolved Hide resolved
apps/antalmanac/src/lib/locations/catalogueBuilder.ts Outdated Show resolved Hide resolved
apps/antalmanac/src/lib/locations/catalogueBuilder.ts Outdated Show resolved Hide resolved
@xgraceyan xgraceyan requested a review from KevinWu098 January 20, 2025 00:32
@KevinWu098
Copy link
Member

I'm trolling, didn't see the re-request, will review asap 🫡

@KevinWu098
Copy link
Member

In the meantime, would you mind resolving the merge conflict (should be very quick)

Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

Very good work! LGTM 🚀

I think that, like a few of our other "generated" data lists (e.g. term data, fuzzy search), locations would also be a good one to generate at build time!

apps/antalmanac/src/lib/locations/catalogueBuilder.ts Outdated Show resolved Hide resolved
Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

Lockfile looks funny ⬇️

pnpm-lock.yaml Outdated Show resolved Hide resolved
@github-actions github-actions bot had a problem deploying to staging-backend February 3, 2025 23:42 Failure
@xgraceyan xgraceyan force-pushed the reformat-buildingCatalogue branch from 0f53db5 to 8bf0cbb Compare February 3, 2025 23:52
@KevinWu098
Copy link
Member

Decided to go in and resolve the conflict -- in the future, deleting the lockfile, marking the merge as complete, then re-installing usually does the trick!

@KevinWu098 KevinWu098 self-requested a review February 4, 2025 08:14
@jotalis jotalis self-requested a review February 4, 2025 08:32
Copy link
Contributor

@jotalis jotalis left a comment

Choose a reason for hiding this comment

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

lgtm

@KevinWu098 KevinWu098 merged commit 0bdad42 into main Feb 4, 2025
4 checks passed
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.

fix: ISEB is not included in buildingCatalogue.ts
3 participants