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.

Sign up for GitHub

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

Save e-learning venues as E-Learning in scraper #2823

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+127 −2
Open

Save e-learning venues as E-Learning in scraper #2823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc2a21b
Fix ECMAScript target in tsconfig
chrisgzf Aug 16, 2020
deec1f6
Save e-learning venues as E-Learning
chrisgzf Aug 16, 2020
eb46b9e
Add tests
chrisgzf Aug 16, 2020
b0514b1
Alphabetical order for tsconfig lib
chrisgzf Aug 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions scrapers/nus-v2/src/tasks/GetSemesterTimetable.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1611,6 +1611,117 @@ Object {
},
],
}
`);
});

// Lessons with E-Learn_* in venue should have E-Learning as venue
test('should save venue of lessons with E-Learning venues correctly', async () => {
const task = createTask(
CS4238Timetable.map((lesson) => {
return { ...lesson, room: 'E-Learn_C' };
}),
Comment on lines +1620 to +1622
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CS4238Timetable.map((lesson) => {
return { ...lesson, room: 'E-Learn_C' };
}),
CS4238Timetable.map((lesson) => ({ ...lesson, room: 'E-Learn_C' }));

);
const timetable = await task.run();

expect(timetable.CS4238).toMatchInlineSnapshot(`
Array [
Object {
"classNo": "1",
"day": "Monday",
"endTime": "2030",
"lessonType": "Lecture",
"size": 40,
"startTime": "1830",
"venue": "E-Learning",
"weeks": Array [
1,
2,
3,
4,
5,
6,
7,
8,
9,
10,
11,
12,
13,
],
},
Object {
"classNo": "2",
"day": "Tuesday",
"endTime": "2030",
"lessonType": "Lecture",
"size": 40,
"startTime": "1830",
"venue": "E-Learning",
"weeks": Array [
1,
2,
3,
4,
5,
6,
7,
8,
9,
10,
11,
12,
13,
],
},
Object {
"classNo": "2",
"day": "Tuesday",
"endTime": "2130",
"lessonType": "Laboratory",
"size": 40,
"startTime": "2030",
"venue": "E-Learning",
"weeks": Array [
1,
2,
3,
4,
5,
6,
7,
8,
9,
10,
11,
12,
13,
],
},
Object {
"classNo": "1",
"day": "Monday",
"endTime": "2130",
"lessonType": "Laboratory",
"size": 40,
"startTime": "2030",
"venue": "E-Learning",
"weeks": Array [
1,
2,
3,
4,
5,
6,
7,
8,
9,
10,
11,
12,
13,
],
},
]
`);
});
});
15 changes: 14 additions & 1 deletion scrapers/nus-v2/src/tasks/GetSemesterTimetable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,20 @@ export function transformModgrpToClassNo(modgrp: string, activity: string): stri
}

export function mapTimetableLesson(lesson: TimetableLesson, logger: Logger): TempRawLesson {
const { room, start_time, end_time, day, module, modgrp, activity, eventdate, csize } = lesson;
const {
room: providedRoom,
Copy link
Member

Choose a reason for hiding this comment

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

This avoids the || '' and ?. below

Suggested change
room: providedRoom,
room: providedRoom = '',

start_time,
end_time,
day,
module,
modgrp,
activity,
eventdate,
csize,
} = lesson;

// Save E-Learn_* venues as E-Learning
const room = providedRoom?.startsWith('E-Learn_') ? 'E-Learning' : providedRoom;

if (has(unrecognizedLessonTypes, activity)) {
logger.warn(
Expand Down
3 changes: 2 additions & 1 deletion scrapers/nus-v2/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
{
"compilerOptions": {
"outDir": "build",
"target": "es2020",
"target": "es2018",
"lib": ["dom", "es2020"],
Copy link
Member Author

Choose a reason for hiding this comment

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

@ZhangYiJiang saw your comment about the target here #2784 (comment), I also added "dom" to lib because the URL() calls were throwing errors in my tests: Cannot find name 'URL'. Did you mean 'url'? Not sure if you've encountered this too but adding "dom" fixed this for me.

Copy link
Member

Choose a reason for hiding this comment

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

That's probably not correct - this code will only be run in Node.js, and while dom lib will add URL as a global like in Node 10/12, it will also add a bunch of other globals like document and navigator which are not valid. The correct way to fix this is to use the @types/node typing, but it doesn't add globals unless it is imported at least once, so in the end it's probably best to just import URL explicitly import { URL } from 'url';

"module": "commonjs",
"baseUrl": "./src",
"resolveJsonModule": true,
Expand Down