-
Notifications
You must be signed in to change notification settings - Fork 80
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
Conversation
…d imports Grouped them into a new folder in src/lib/locations
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.
Great improvement to our system! Just some minor suggestions, but overall very thoroughly put together!
I'm trolling, didn't see the re-request, will review asap 🫡 |
In the meantime, would you mind resolving the merge conflict (should be very quick) |
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.
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!
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.
Lockfile looks funny ⬇️
0f53db5
to
8bf0cbb
Compare
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! |
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.
lgtm
Summary
catalogue_builder.js
intocatalogueBuilder.ts
locations.ts
alongsidebuildingCatalogue.ts
locations.ts
andbuildingCatalogue.ts
with its recently updated versioncatalogueBuilder.ts, buildingCatalogue.ts, locations.ts
intoapps/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, andJSON.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
locations.test.ts
and make sure all tests pass (all tests passed when I ran it on my end)Issues
Closes #1085