-
Notifications
You must be signed in to change notification settings - Fork 344
Change representation of lesson configs from classNo to lesson indices #4225
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@zehata is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4225 +/- ##
==========================================
- Coverage 54.52% 54.38% -0.14%
==========================================
Files 274 289 +15
Lines 6076 6862 +786
Branches 1455 1675 +220
==========================================
+ Hits 3313 3732 +419
- Misses 2763 3130 +367 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changes from #4226 is only a bugfix. The delta from this PR will apply to the fixed master too. |
Implementation
["CS1010S"]
classNo
lesson configs and TA lesson configs will be migrated to the newlessonGroup
format(..., ...)
denotes arrays of lesson indices?CG1111A=LAB:03
?CG1111A=LAB:(5,6)
;
separates different lesson types?CS1010S=LEC:1,TUT:1
?CS1010S=LEC:(53);TUT:(41)
?CG1111A=LAB:03&ta=CG1111A(LAB:03)
?CG1111A=LAB:(5,6)&ta=CG1111A
lessonGroup
format.Resolves
All modules can now be set as TA modules.
Resolves #3929 and #3930 , from which this implementation came from
Issue with deserialization of TA module lesson configs with multiple lessons in a module, from #4214 (comment).
All lessons of a single classNo now show up, and are highlighted when one of them is hovered over. Provided that the module is not set as a TA mod. Resolves #4168.
TA module lessons can now be added and removed individually.
User request from: https://t.me/NUSMods/12508 (No issue opened, at least I can't find one)
Known issues
Array Index Stability
Using the index of an array comes with the inherent issue of stability. Furthermore, it may fail silently, since lessons of the same lesson type are typically placed next to each other in the timetable list in the API response. So the module lesson config may remain valid even after the module timetable had been updated.
An alternative is to serialize additional info as proposed in #3930 . It will make share links very long, because of the serialization of weeks information.
Personally I think the lessons provided by the faculties are stable enough, and they will most likely not add an additional class in the middle of the semester, so I think it will be better idea to create UI to inform users that the module info has changed if it changes, which can be done simply by hashing the module info during scraping.