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

xgraceyan · 2025-01-15T02:22:56Z

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

…d imports Grouped them into a new folder in src/lib/locations

KevinWu098

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

apps/antalmanac/src/lib/locations/catalogueBuilder.ts

KevinWu098 · 2025-01-15T09:25:54Z

apps/antalmanac/src/lib/locations/catalogueBuilder.ts

-const fs = require('fs');
+import fs from 'fs';
+
+import axios from 'axios';
 // https://map.uci.edu/?id=463#!s/?ct/8424,8309,8311,8392,8405,44392,44393,44394,44395,44396,44397,44398,44400,44401,44402,44538,44537,44399,8396,11907,8400,10486,11906,11889,8310,8312,8393,8394,8397,8398,8399,8404,8407,8408,11891,11892,11899,11900,11902,21318,8406,11908,11935


chore: maybe colocate this with our CATEGORIES const to explain better where we source our category ids from. adding a brief comment to further illustrate this would be good too

KevinWu098 · 2025-01-15T09:33:43Z

apps/antalmanac/src/lib/locations/catalogueBuilder.ts

+                : location.name;
+            locationIds[locationName] = location.id;
+
+            await sleep(250);
        }
    }
 }

 (async () => {


issue: looks like this code works, but it's a tad hard on the eyes to read.

suggestion: rewrite this function as a main() and call that at the bottom of the file. Further, break up the string literals into separate variables

const foo = ` export interface Building { imageURLs: string[]; lat: number; lng: number; name: string; }\n\n `;

KevinWu098 · 2025-01-15T09:36:39Z

apps/antalmanac/package.json

@@ -39,6 +39,7 @@
        "@tanstack/react-query": "^4.24.4",
        "@trpc/client": "^10.30.0",
        "@trpc/server": "^10.30.0",
+        "axios": "^1.7.9",


Suggested change

"axios": "^1.7.9",

issue: would be nice to not add an additional dependency; we can do this with just fetch

KevinWu098 · 2025-01-15T09:36:57Z

apps/antalmanac/src/lib/locations/catalogueBuilder.ts

-const fs = require('fs');
+import fs from 'fs';
+
+import axios from 'axios';


repeat: lets not use a new dependency; fetch is fine here

KevinWu098 · 2025-01-15T09:40:55Z

apps/antalmanac/src/lib/locations/catalogueBuilder.ts

    return new Promise((resolve) => setTimeout(resolve, ms));
 }

 async function fetchLocations() {
    // Hits locations API to get a list of every single location
-    let locations = await axios.get(LOCATIONS_LIST_API);
+    let locations: any = await axios.get<BuildingLocation>(LOCATIONS_LIST_API);


issue: using any explicitly and naming both the response and the resulting data the same variable can create a surface for potential errors

suggestion: aside from switching out Axios for Fetch, I'd recommend naming the variables more clearly and not using any. When you must make an assertion (which you'll have to), I'd recommend asserting the type of the decoded data.

Would also be good to write some validator for the data, but I think we can skip on that for now.

KevinWu098 · 2025-02-01T02:12:16Z

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

KevinWu098 · 2025-02-01T02:12:53Z

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

KevinWu098

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!

KevinWu098 · 2025-02-02T00:30:50Z

apps/antalmanac/src/lib/locations/catalogueBuilder.ts

+        );
+
+        // If one of the locations belongs to one of the categories above, and is not a duplicate
+        if (CATEGORIES.has(location.catId) && !locationExists) {


nit (non-blocking): An (arguably) better way to write this function would be with a few early returns/guard clauses

if (!CATEGORIES.has()) { return; } if (!locationExists) { return; } // do logic after guard clauses ...

KevinWu098

Lockfile looks funny ⬇️

KevinWu098 · 2025-02-02T00:36:59Z

pnpm-lock.yaml

Hmm... this lockfile looks wrong, actually...

Why are we removing so many dependencies? Can you rerun pnpm install at the root and commit the resulting file? If it's the same, just lmk

xgraceyan added 5 commits January 12, 2025 17:27

Reformatted catalogueBuilder, buildingCatalogue, locations and update…

06bae58

…d imports Grouped them into a new folder in src/lib/locations

Update duplicate check in catalogueBuilder.ts

1f907f1

Updated dependencies for axios

5795e13

Added types to a few variables

f86d758

Refactor buildingCatalogue variable name

3feebdb

github-actions bot deployed to staging-frontend January 15, 2025 02:25 View deployment

KevinWu098 requested changes Jan 15, 2025

View reviewed changes

xgraceyan added 2 commits January 19, 2025 16:28

Removed axios dependency + made requested changes to catalogueBuilder

c829321

Reset pnpm lock (removed axios dependency)

87f1837

xgraceyan requested a review from KevinWu098 January 20, 2025 00:32

KevinWu098 approved these changes Feb 2, 2025

View reviewed changes

KevinWu098 requested changes Feb 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xgraceyan commented Jan 15, 2025

KevinWu098 left a comment

KevinWu098 Jan 15, 2025

KevinWu098 Jan 15, 2025

KevinWu098 Jan 15, 2025

KevinWu098 Jan 15, 2025

KevinWu098 Jan 15, 2025

KevinWu098 commented Feb 1, 2025

KevinWu098 commented Feb 1, 2025

KevinWu098 left a comment

KevinWu098 Feb 2, 2025

KevinWu098 left a comment

KevinWu098 Feb 2, 2025

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

Are you sure you want to change the base?

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

Conversation

xgraceyan commented Jan 15, 2025

Summary

Important note/potential issue

Test Plan

Issues

KevinWu098 left a comment

Choose a reason for hiding this comment

KevinWu098 Jan 15, 2025

Choose a reason for hiding this comment

KevinWu098 Jan 15, 2025

Choose a reason for hiding this comment

KevinWu098 Jan 15, 2025

Choose a reason for hiding this comment

KevinWu098 Jan 15, 2025

Choose a reason for hiding this comment

KevinWu098 Jan 15, 2025

Choose a reason for hiding this comment

KevinWu098 commented Feb 1, 2025

KevinWu098 commented Feb 1, 2025

KevinWu098 left a comment

Choose a reason for hiding this comment

KevinWu098 Feb 2, 2025

Choose a reason for hiding this comment

KevinWu098 left a comment

Choose a reason for hiding this comment

KevinWu098 Feb 2, 2025

Choose a reason for hiding this comment